Daniel Convissor [2012-10-22 14:30 UTC] Hi Pranjal:
Why are there a bunch of files containing individual functions rather than having those functions be methods inside some class? Even if they need to be standalone functions for some technical reason, they should be in one file because loading files is very resource intensive process.
Thanks,
--Dan
Till Klampaeckel [2012-10-22 18:47 UTC] Hi,
as Daniel noted — please use an object oriented approach. A good idea would be to review some popular components which are already part of PEAR.
We are happy to help and give feedback if you're stuck.
Let us know,
Till
Pranjal Prabhash [2012-10-25 18:17 UTC] Hi,
I have tried to reduce the number of files from 15(initially) to 8(currently). I have added few functions as methods of classes, for others, I have put them in a new file methods.php. These functions in methods.php cant be put into classes because they are responsible to do various tasks like parsing of xml, decoding objects to and from, identifying objects and various others.
Thankyou,
Pranjal Prabhash
Till Klampaeckel [2012-10-25 19:11 UTC] Additional comments on your latest changes:
1) The helper methods should go into an object then.
2) Try to make objects injectable – e.g. that instance of HTTP_Request2. It's good that you're using other PEAR components but the way you use it means that it cannot be configured by the user of your library.
They may need a proxy to connect, they may want to use cURL, they may want to set a timeout, etc..
3) Avoid constants in the global scope and provide configuration to the classes via injection: https://github.com/pranjal710/osrf/blob/master/lib/Config.php – configuration should be given to the classes via parameters. People typically install PEAR code in some system wide directory and they shouldn't be required to edit files in there.
4) Avoid func_get_args() — define parameters in the method's signature and use them accordingly.
5) Avoid including inside a class – if necessary do it before the class body. But it would be much nicer to use autoload.
6) Various coding style things — the most obvious is stacking your classes into a common namespace (maybe "OSRF").
PEAR uses the File-ClassName-convention. This means: lib/OsrfSession.php would be lib/OSRF/Session.php. For all other coding style issues, please run phpcs (PHP_CodeSniffer) on your codebase.
7) It would be nice to see some test cases.
8) Code execution in the global scope: https://github.com/pranjal710/osrf/blob/master/lib/Fieldmapper.php#L51-L69 — It's also not very 'defensive'. When one of those calls fails, it'll fail with E_WARNING, E_NOTICE and possible E_FATAL. Libraries need to be robust and return a clear status to the user.
|
