[PEPr] -1 for Web Services::Livejournal

Justin Patrin (http://pear.php.net/user/justinpatrin) has voted -1 on the proposal for Web Services::Livejournal. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=360 Vote information: http://pear.php.net/pepr/pepr-vote-show.php?id=360&handle=justinpatrin Comment: You really should have waited for further comments. I hadn't looked closely at your code before as you had lots to change. I have far too many more comments to justify a +1 right now. One comment, however large, really isn't enough to justify a call for votes. I'm voting -1 for now as I'd like to see the remainder of these problems fixed before a call for votes. Assuming this proposal fails, create a new proposal and wait for more comments before a call for votes. Ask on the PEAR-dev list before you call for votes and wait. Yet again, I still have not looked at all of your code as I found plenty in the first 3 or 4 files to comment on. Your indenting is still funky. Look at your source online. Most of your throw commands are not indented right, for example. Instead of parsing an ini file internally use a static property. This will allow users to use an ini file, an ini file with sections (for multiple packages) or even a normal PHP array. Something like: static public $options; or static private $_options; and Services_LiveJournal::setOptions($arr); Or, if the option info needs to be configurable per-instance, leave the options as a private var but allow the user to set the options as an array in the constructor instead of passing in a filename. Put the userInfo from DB errors in the exception's message. If you don't include iserInfo it's far more complicated to figure out what went wrong most of the time. ($err->getUserInfo()) In array declarations (such as $_defaultSettings) put each key/value pair on its own line for readability. Don't use array_pop(explode('_', $className)), use substr($className, strrpos($className, '_') + 1). Don't use () with require_once. I suggest using only _method_map and not _object_map. Use an associative array instead of two arrays and searching. $this->_method_map[$method->name] $object. You don't need this: if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') { $slash = '\'; } else { $slash = '/'; } Use DIRECTORY_SEPARATOR instead. I'm really not sure what the code after this comment is for: // two folders, one file back It looks a little complicated for something as simple as setting a data folder. It looks like this is meant to get the folder where configuration files may be kept. This should not be done in this way. Use the PEAR installer's replacement tasks to replace something like @data_dir@ with the PEAR data directory. Any "default" config files should be put in this folder. In addition, if this is something which you want to be configurable (it seems like it should be) then you should allow its setting in the constructor and/or in the main options array. array_shift(split('_',$name,2)) also seems a bit wrong to me. User substr and strpos. I'm not sure I like the way that getConfigValue is reading from a file every time the config value may be used. Are these files ever used multiple times in one script run? If so, it may make more sense to cache the file contents. Don't use escapeSimple(). Use quoteSmart() and remove the hard-coded ' around the values. ALWAYS escape output when putting it in HTML. Within an attribute you need to use htmlentities($val, ENT_QUOTES). Within normal HTML just htmlentities() is fine. (see _formatUserInfo()). NOW() is a mysql-ism. If you leave your call as NOW() then your code is going to be tied to certain databases. Again, $this->_memories[(string)$row[0]] really isn't going to help. You don't need the (string). You have these all over the place. ++$var is more efficient than $var++ Don't silence calls unless you have a compelling reason. @$this->_memories[(string)$id] is not a compelling reason. Check for it being set with isset() first. @constant('UTF-8') is also not a compelling reason. If UTF-8 is not always defined then use is_defined() first. Don't use function_exists($callback), use is_callable($callback). This allows for method callbacks. Use DB's auto* functions more often instead of hard-coding so much simple SQL. http://pear.php.net/manual/en/package.database.db.intro-auto.php In addition, prepared statements may save you some runtime if you have to run the same SQL over and over. -- Sent by PEPr, the automatic proposal system at http://pear.php.net