Comments for "Services_Memotoo"

» Submit Your Comment
Please log in to enter your comment. If you are not a registered PEAR developer, you can comment by sending an email to pear-dev@lists.php.net.
» Comments

  • Michael Gauthier  [2010-01-17 04:58 UTC]

    In general the code looks good. Here are some suggestions:

    1.) @ error suppression is a bad idea. Use exception handling for the SOAP object instead.

    2.) Consider using protected members rather than private. Protected members are accessible to subclasses and can make testing easier.

    3.) self::ERROR_SOAP is undefined and will cause a notice when the exception is thrown.

    4.) It's a bit strange to have debug() in the public API.

    5.) Summaries for methods should use 3rd person declarative rather than 2nd person imperative, beginning with a verb phrase. (Gets X, rather than Get X).

    6.) It might make sense to split the classes up into separate groups like bookmarks, tasks, contacts, calendar, etc. The API would then read like: $memotoo->getContactGroup('friends')->search('Joe');

  • Daniel O'Connor  [2010-02-08 13:57 UTC]

    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.