Vote Details for "MDB2_TableBrowser" by till

» Details
» Comment
Hey there,

first off, thanks for improving the class and implementing it with MDB2.

Here is my (quick) feedback which makes this vote conditional:

1) Even though you spent a lot of time documenting inline, there's room for improvement (see "8").

2) Fix the package.xml (and use "pear package" to create a release, also, your first version should not be 1.0 and also not stable. ;)) Also add dependencies, such as MDB2 etc. to that file.

3) When you add a limit to the query, please explore MDB2's setLimit() to make your code portable.

4) The Exception class should extend PEAR_Exception, and should also go in its own File.

5) Follow the naming convention, in your repository you want:
MDB2/
MDB2/TableBrowser.php
MDB2/TableBrowser/DBException.php

The above assumes your classes are called:
MDB2_TableBrowser
MDB2_TableBrowser_DBException

6) This is a minor, but I believe when you wrote "Santize" (e.g. dbSantizeIdentifier(...)), you meant "Sanitize".

7) In your exception class, I like how you pass in the PEAR_Error (well, essentially MDB2_Error), what I'd like to see is that you extend and take more information then the "getMessage()" into the exception - e.g. the error code, maybe getDebugInfo() etc..

With no offense (to MDB2 or anyone working on it :)), but some MDB2 error messages are pretty plain and I found e.g. getDebugInfo() to be way more helpful in most situations. IMHO it would be less useful for the developer if that information is lost when you transfer the MDB2_Error to your exception.

8) Run "phpcs" (http://pear.php.net/package/PHP_CodeSniffer) on your code, this will help you to get your code up to PEAR coding standards and also let you know where and how to improve your documentation.

9) Since (I think) you always check the table definition, maybe add support for auto discovery of the PK (in case none is supplied). :)

10) Try to exit early (with return), vs. having huge if/else statements in your code:

For example:
function foo()
{
if ($whatever) {
return;
}
return false;
}

11) Instead of dbDoSelect(), maybe you want to look into the MDB2 shortcuts getAll(), getRow(), getOne() etc. - wherever they make sense.

12) Last but not least, make sure to change a license for the code. Many people swear by BSD, MIT or LGPL licenses. :)


One final question, difference of your factory vs. ctr - the ctr allows me to use my own MDB2 connection, correct?

I liked the example a lot (though I did not run it). MDB2_TableBrowser looks pretty easy to work with, and I look forward to it.

Hope I didn't oversee anything. Didn't have too much time.
If you have questions, or whatever, feel free to get in touch.

Cheers,
Till