Michael Gauthier [2008-04-21 03:02 UTC] Code looks good in general. You should run the code through phpcs (http://pear.php.net/package/PHP_CodeSniffer) and fix any errors it finds. It will prevent nitpicky coding standards issues from holding your package back from acceptance in PEAR.
Instead of catching exceptions and echoing a message you should throw a meaningful exception.
Some method documentation is a little sparse. For example, the documentation for the method addBookmarkFolder is: Add bookMarkfolder. This may make sense to you but doesn't mean anything to someone else trying to use the method.
@access tags should not be used for PHP5 code where method/member variable scope is specified in the declaration.
Philippe Jausions [2008-04-21 03:19 UTC] On top of Michael's comments, why not store $this->password with the UTF8'd value instead of calling _convertUtf8() every time, same for login.
BTW: you probably have a typo in _convertUtf8() with $texte instead of $str
Till Klampaeckel [2008-04-21 17:25 UTC] Here are some suggestions:
* You probably don't want to start with 1.0.0 (and stable)
* You could simplify your if/else statements (when they "return").
* You shouldn't use the PHP license as it is incompatible with the GPL
(see LGPL or BSD instead).
* Are all examples listed in package.xml really tests?
* You need to include the package.xml in your tarball (use "pear package" to create it).
* As suggested, please use exceptions (Services_Memotoo_Exception extend PEAR_Exception).
* Use phpcs to address CS issues.
* It doesn't hurt to include the <changelog> in your revisions. ;-)
* This is a suggestion, so if you need to include a debugger, maybe think about accepting a logger instance into your class (optional) or create one from scratch if none is supplied - this is more helpful in production - at least instead of "echo'ing" data.
Questions:
1) Does Memotoo adhere to any standard API-wise? Like is it compatible with other addressbook services (for example, such as Zyb, Plaxo, Google, ...)?
2) On a related note, does Memotoo have a syncml api? I saw that it lists all kinds of devices on the website, but I couldn't figure out how they sync.
Thanks!
Ken Guest [2008-04-21 22:15 UTC] There are a staggering amount of phpcs highlighted issues in Memotoo.php, close to 3000 in fact that I'm not sure I want to look at the code - to be fair it's just a case of working to the PEAR Coding Standards and is not a major issue at this stage.
It's good to see such a large number of examples too though.
I'd be inclined to rethrow exceptions (and perhaps others) rather than display the error messages in the constructor, and in other methods of your API. By doing this you'll give developers using your API the choice to decide how to handle (and trap) those scenarios better.
Pequet Thomas [2008-04-22 16:50 UTC] Thanks for your comments :-)
Ok I have modify as you say:
- add better documentation for the method
- correct error with phpcs (I have just a warning with php version ?)
- write the error in a file if you want, or display these in the browser
- now Apache License
- ...
Yes all the tests files have been tested.
Till:
1) Memotoo have other API: iCalendar for calendar, vCard for contact, Xbel for bookmarks, SyncML for mobile phone, WebDAV for files, ... (more information: http://www.memotoo.com/index.php?rub=api).
For the moment, Memotoo can be compatible manually with other addressbook services, and I think soon integrate gateway between these websites.
2) Yes Memotoo have a SyncML api, you can found more information here: http://www.memotoo.com/index.php?rub=infoSyncML
|
