Vote Details for "Date_HumanDiff" by gauthierm

» Details
Voter: Michael Gauthier Vote: +1 (conditional) Reviews: Deep source review
» Comment
My vote is conditional as I have a few concerns: 1.) Method name setLocale() instead of setLanguage() makes more sense to me as you are passing in locales. I'd rename Lang classes as Locale at the same time. 2.) Does the sort method need to be static? I'd make it protected and dynamic so it can be overridden in subclasses. 3.) In makeTimestamp you use if ($foo) { return 'a'; } else { return 'b'; }. The else is redundant. Two separate if statements might be clearer. 4.) isIncludable() has error suppression operator which is not allowed in PEAR. Can you use file_exists + is_readable instead? 5.) Class docs for the Lang classes would be nice. 6.) What about languages with multiple plural forms? Will this work with Slocak or Arabic? Should this library even be responsible for translating strings? Another library like gettext or Zend_Translate seems like a better place to handle that. 7.) Regarding the trailing ?> IIRC you're supposed to use it in PEAR 1. PEAR2 says you must not. It's not a blocker issue for me either way.