[PEPr] Comment on PHP::opensrf-php

[email protected] ("Till Klampaeckel")
Newsgroups php.pear.dev
Message-ID <[email protected]>
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.

-- 
http://pear.php.net/pepr/pepr-proposal-show.php?id=681
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.