Philippe Jausions [2008-02-09 19:37 UTC] Oh, boy...
Let's start with the educational stuff:
- fms_if() try ?: ternary operator instead.
- Use PHP_CodeSniffer package to fix all the PEAR Coding Standard issues.
- Do not pollute the global namespace with functions, use static method instead.
- Do not define global constants for options, that prevents from changing the behavior of the class from one instance to the other.
- Don't use eregi* functions, use simple string or preg_* functions.
- Leverage existing packages if possible, i.e. MIME_Type, File for instance.
- Don't EVER use eval() (at least not until you have a LOT more experience). Learn a bit more about variable variable and variable function first.
- Use exceptions instead of always returning FALSE on error (FALSE doesn't provide much information, whereas an exception can)
- Use __call() for your cmd(). Why a use an error-prone string parsing-based approach when the rest is OOP?
- Default a parameter to NULL instead of FALSE
Things like:
if (SHOW_SECRET_FILES === FALSE && $result{0} != '.') {
$files_stats++;
} elseif (SHOW_SECRET_FILES === TRUE) {
$files_stats++;
}
are much simpler written:
if (SHOW_SECRET_FILES || $result{0} != '.') {
++$files_stats;
}
Now, have you looked at SPL?
Seriously, as-is, the package is not PEAR-quality. Rework it around SPL, learn the PEAR coding standard, avoid pitfalls like eval(). I'd also suggest to look at existing PEAR packages and how you could contribute to them for some convenience methods.
Torsten Roehr [2008-02-10 19:37 UTC] This seems to be more an application than a library. What do others think?
Clay Loveless [2008-02-10 19:43 UTC] I think that if it is "library-ized", and the issues that Philippe highlights are resolved, it could be considered.
... unless PEAR has evolved to a point where the 'A' in the acronym is just there for show? :)
Jaco [2008-02-16 14:44 UTC] Thanks to all of you for your advice, I tried to improve my code according to your directions.
I hope that it can be useful
|
