[PEPr] Comment on Web Services::Services_Memotoo

Hey Pequet, thanks for the proposal! Couple of nit picks: 1) Any unit test coverage around? http://www.phpunit.de/ is your friend. 2) You might be better off with doing dependency injection: IE; you constructor is doing a bunch of work, and you can't put a fake soap client in (good for unit tests). public function __construct($login, $password, $useHttps = false) { $this->_login = $this->convertUtf8($login); $this->_password = $this->convertUtf8($password); $this->_useHttps = $useHttps; try { $this->soapClient = @new SoapClient( ($this->_useHttps ? "https" : "http"). '://api.memotoo.com/SOAP-server.php?wsdl', array('trace' => 1) ); } catch (Exception $e) { throw new Services_Memotoo_Exception($e->faultstring, 1); return ; } } Why not just public function __construct(SoapClient $soap_client, $options = array('username' => null, 'pass' => null)) { ... } or something similar? That way, if the soapclient doesn't load the WSDL, I as the developer have a lot more control over it. I can also just make fake soap clients which give back canned data, and dont rely on the network being available for the code to work. http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ might also be worth a quick read. 3) try {} catch ($e) { throw $e } is a bad idea, in that you are capturing everything, removing backtrace information, and then rethrowing it. I would suggest allowing the soap fault to bubble up, and documenting the possibility it may be raised. 4) As an overall library, what's compellingly more useful about your implementation over simply loading a soap client, wsdl, and relying on PHP's inbuilt soap magic? At the moment it looks like a bit of syntax sugar, parameter building, UTF conversion and exception handling for you - is there anything else in there? Overall, I'd be happy to see this in PEAR if it was unit tested, and the purpose was changed a little bit - perhaps to make it a collection of utilities/objects that make interacting with the soap client trivial; rather than a soap client wrapper. For example, in a lot of the docs: http://www.memotoo.com/softs/?page=nav&contenu=dossier&dossier=PEAR_Services_Memotoo%2FMemotoo%2Fexamples&sort=nom&action=afficherfic&fic=addBookmark.php You are essentially asking me as the user to create a very simple hash with data which may or may not be valid. $arrayBookmark = array( '0' => array( 'url' => 'http://www.google.fr', 'description' => 'Search engine', 'tags' => '', 'rank' => '4', 'id_bookmarkFolder' => '0', ), ); Why not a Bookmark class, which has a toArray(), setters, getters, simple validation? And a Services_Memtoo_DataSanitizer, which hands all of the current convert to UTF stuff (same code, just shifted into a different objcet, and baked into your data objects) As a consumer of your API, your data objects are the hard bit to understand/validate/convert to UTF - not so much the soap client bits. -- http://pear.php.net/pepr/pepr-proposal-show.php?id=620