Feedback/review on DBA class wanted.

Seeing as you have this wonderful section here, I was hoping that I could get some knowledgeable people to help me look over a DBA layer class I've been working on?

I'm looking for feedback on all aspects of it, and all constructive criticism would be highly appreciated. Whether it be negative or positive. So please don't be shy about commenting, as long as you can explain your position.

In case it's not obvious it's meant as a fully interchangeable class for interfacing against a multitude of DBMS, without having to change anything other than the name of the file included. It doesn't, obviously, support all of the features. Nor will it ever do, but it should support the most used ones. If I've missed some, please let me know as well as why you think it's a necessary inclusion.

You can find the class here: http://pastebin.com/tJYNiBKE

And some examples on how it can be used: http://pastebin.com/aygrZJTa

It exists primarily because of 4 reasons, in order:

[*]I wanted to learn more about DBA, and what it takes to write a good class for it.

[*]Other classes I've looked at didn't give me what I really wanted, either due to lack of features, too high usage complexity, not being DBA but DAA classes and so forth.

[*]I had an old attempt, that didn't work as an abstraction layer for anything but the most simple of queries, and wanted to improve it.

[*]And a little bit of "because I could."

I also got asked "why not PDO, as it gives you the same".

First and foremost it's because PDO isn't a DBA (database abstraction) class, but a DAA (data-access abstraction) class. They even mention it in the intro for PDO. So, it does not give me the same.

There are a few other things that I don't like about PDO as well, such as the throwing of exceptions for everything (I blame MSStrings for my hate of this), the connection string and a few other caveats. Arguably most of these caveats I have are personal preference, I have to admit. Still, that doesn't change the fact that PDO isn't a DBA layer.

PS: If anyone wants to use it for their own projects, the license is stated in the PHPdoc header for the class. Should anyone, for any reason, want to use it in a commercial project please let me know. I'm sure we can come to an agreement.

PPS: I posted this on TheDailyWTF forum as well, though the response was rather lacklustre unfortunately.

1. if (!class_exists('SQL_Conn')) { That should not be there. If you want to avoid problems with require(). You can also autoload your class.. After watching your example I think I get what you are trying to do. You should create an interface/abstract class and create separate classes for each of the different DBMS. Take a look at the Adapter pattern.

2. I would make those constants part of SQL_Conn and drop the DB_ prefix.

3. SET NAMES 'utf8' You should be using mysqli_set_charset() for this. I would also make this configurable in case you would use some different charset.

4. Show_Error() Not really a fan on using custom global functions inside a class. It hurts the re-usability of your class. I would make it either that the class handles the error or throw an Exception. Should the client always define this function before including the class? or should it be part of the class?

There's a lot of code, I haven't gone over all of it.

First of all, thanks for taking the time.

Then to your remarks:

1. [*]I've done this to ensure that there is no way there can be a naming collision, regardless of how the class is included. Also, it'll prevent collisions with other classes as well.

Something I've picked up from Qt. Call it "Defensive Programming", if you like.

[*]I'll look into this. Primary reason I've done as I've done is to be able to use the constants like the flags for "preg_match_all ()" and other built-in functions.

[*]You're right, and I'll fix this. Thanks.

[*]As noted in my first post, I'm not a too big a fan of throwing exceptions. Especially for what can be considered "normal" error conditions. I do, however, agree that relying upon a custom error function does increase the effort needed in order to import this. The first time, at least.

Currently the error function used here is a part of my framework, and ties nicely into my template engine. Which works quite nicely, for me at least. That said, I'm not quite 100% satisfied with the current solution, and I'll take your comments into consideration.

Will have a look at the Adapter pattern as well, see if it fits my intentions. Though, even if it does I won't be removing that first line.

1. Things like double-including or re-declaring a class SHOULD produce errors. You don't want to silently ignore, because that's allowing for bad programming practise. Hiding errors isn't defensive programming, it's making it harder to debug when you screw up (or to know if you've screwed up in the first place).

2. Constants in classes are static, so as long as the class exists in a given scope, so do the constants. Putting them in the class just gives them an automatic prefix, and makes it more obvious that those constants relate directly to that class.

4. Use an error class instead, so you can autoload. Making it a static class will allow you to maintain state throughout your entire script, but won't solve tight coupling issues. When it comes to errors and logging though, this doesn't bother me as much. I think you need to look into exceptions more, as they are quite lovely. By using multiple exception classes, you can try/catch specific errors, and handle them a different way. You can have an exception class for 'normal' errors, and one for 'fatal' ones. As long as the try/catch blocks are specific to a single exception (not an inherited one), they won't catch other throws.

<?php

class minor extends Exception {}
class major extends Exception {}

try {

if(rand(0,1))
	throw new minor('minor thrown');
else
	throw new major('major thrown');

} catch( Exception $e ) {
// This will catch an exception from EITHER minor or major
echo $e->getMessage().'<br>';
}

try {
try {

	if(rand(0,1))
		throw new minor('minor thrown');
	else
		throw new major('major thrown');

} catch ( minor $e ) {
	echo $e->getMessage().'<br>';
}
echo 'this will still execute if minor was thrown, but not if major';

} catch ( major $e ) {
echo $e->getMessage().'<br>';
}

?>

I'll look into this. Primary reason I've done as I've done is to be able to use the constants like the flags for "preg_match_all ()" and other built-in functions.

There is a big difference. preg_match_all() is a function, not a class and thus can't create class constants.

Though, even if it does I won't be removing that first line.

Now you are just being ignorant. Look at other popular established frameworks none of them uses a if (!class_exists('SomeClass')) {. You can name your classes specifically towards their goal class SQL_Conn_MySQLi extends SQL_Conn_Base { now using a beautiful thing called polymorphism I can make my code abstract of the DBMS public function createNewUser(SQL_Conn_Base $conn, $data) {.

To create a new user in MySQL I would do $this->createNewUser(new SQL_Conn_MySQLi, $userdata); to write this same data to a PostGreSQL DBMS $this->createNewUser(new SQL_Conn_PostGreSQL, $userdata);. The function could look like public function createNewUser(SQL_Conn_Base $conn, $data) { $conn->insert('users', $data); }. See, the code inside createNewUser() does not have to be altered yet works with every possible DBMS.

1. Things like double-including or re-declaring a class SHOULD produce errors. You don't want to silently ignore, because that's allowing for bad programming practise. Hiding errors isn't defensive programming, it's making it harder to debug when you screw up (or to know if you've screwed up in the first place).

Yeah, agreed. Stupid to hold on to that, so I've removed it now. Thanks.

2. Constants in classes are static, so as long as the class exists in a given scope, so do the constants. Putting them in the class just gives them an automatic prefix, and makes it more obvious that those constants relate directly to that class.

This is true, and I'm considering whether or not I should use them.

While I prefer the simple syntax I've used now as opposed to SQL_Conn::CLEAR_ALL, as it's shorter to type, you do have an excellent point about the obviousness of where they belong. Thus I'm leaning towards changing this to class constants at the moment, but will deliberate a bit more with myself before making the final decision.

4. Use an error class instead, so you can autoload. Making it a static class will allow you to maintain state throughout your entire script, but won't solve tight coupling issues. When it comes to errors and logging though, this doesn't bother me as much. I think you need to look into exceptions more, as they are quite lovely. By using multiple exception classes, you can try/catch specific errors, and handle them a different way. You can have an exception class for 'normal' errors, and one for 'fatal' ones. As long as the try/catch blocks are specific to a single exception (not an inherited one), they won't catch other throws.

I know what Exceptions are, how to use them and how to extend them. I'm not just a big fan of them¹.

Will play around with the idea of an error class though, seeing as my error handling code is the next item on the list of things to update. Need to find the simplest and most functional way on how to do it, after all. Not that I don't hear what you're all saying about "use exceptions", so I'm not completely against being convinced otherwise.

I should perhaps note that the "Show_Error ()" function is only used for "fatal" errors, and it kill further parsing of the current page. "Soft" errors, such as failed validation, no results returned, and similar are all handled my the calling code and not by "Show_Error ()".

There is a big difference. preg_match_all() is a function, not a class and thus can't create class constants.

See my reply to xyph's second point.

Now you are just being ignorant. Look at other popular established frameworks none of them uses a if (!class_exists('SomeClass')) {.

Yeah, xyph had already convinced me about the stupidity of it, so that's removed now.

You can name your classes specifically towards their goal class SQL_Conn_MySQLi extends SQL_Conn_Base { now using a beautiful thing called polymorphism I can make my code abstract of the DBMS public function createNewUser(SQL_Conn_Base $conn, $data) {.

 

To create a new user in MySQL I would do $this->createNewUser(new SQL_Conn_MySQLi, $userdata); to write this same data to a PostGreSQL DBMS $this->createNewUser(new SQL_Conn_PostGreSQL, $userdata);. The function could look like public function createNewUser(SQL_Conn_Base $conn, $data) { $conn->insert('users', $data); }. See, the code inside createNewUser() does not have to be altered yet works with every possible DBMS.

I am quite aware what polymorphism is, and how to utilize it. When I get 2 or more versions of this class I'll look into refactoring them to utilize polymorphism, if that's desirable. As things are standing now, however, I don't see much point in it.

The naming convention is also somewhat of a bikeshedding issue, which reduces into "should you change the include, or the object instantiation?" I've chosen the include/require statement, because you usually only have one of those, while you can have many object instantiations.

¹ I suspect you need to have used msstrings in Visual C++ to understand why though.

While I prefer the simple syntax I've used now as opposed to SQL_Conn::CLEAR_ALL

Inside the class you can access these constants like self::CLEAR_ALL. No need for all that extra typing.

I know what Exceptions are, how to use them and how to extend them. I'm not just a big fan of them

You don't have to use Exceptions per se anything else is good too as long as it does not involve a global function. Turn your Show_Error() function into an object for example and pass that to your class $this->error->Show();. That will make the dependency more visible than a stowaway function.

Inside the class you can access these constants like self::CLEAR_ALL. No need for all that extra typing.

Yes, but SQL_Con::Clear () is a public method. Not to mention all the other methods that can take the constants as a parameter.

You don't have to use Exceptions per se anything else is good too as long as it does not involve a global function. Turn your Show_Error() function into an object for example and pass that to your class $this->error->Show();. That will make the dependency more visible than a stowaway function.

Ah, yes of course. Thanks for reminding me about that solution, should have thought about it myself tbh.

Will play around a bit with it, and see how it works out in the next version of the class.

After thinking about stuff for a while, I've redone the class a bit based upon the previous feedback. New version can be found here:

MySQL class: http://pastebin.com/5K5VKjZu

Usage examples: http://pastebin.com/NkwETqq0

Note that I've chosen to use trigger_error () with a custom error handler, instead of exceptions, as I found it to be the most flexible manner I could deal with error conditions. It also ties in nicely with the rest of my framework, which doesn't hurt either.

I have also moved the JOIN_TARGETS constant inside the class, as it really didn't need to be used outside. (Ignore the DB_ part of its name, as I just forgot to remove it before uploading it to Pastebin.org.) The other constants will not be moved, as they need to be reachable outside of the class in an easy manner.

Lastly I've changed the name of the class, to support using different versions at the same time. (Might change it into a factory class, if I get more DB flavours added.)

Any feedback would be much appreciated, especially if someone would be kind enough to take the time to play around with it and see how it works in practise.

Those constants are DB specific thus I see no reason why they should be excluded. You can still access them, all you need to do is prefix ClassName::SOME_CONSTANT or if you have an instance of the class $class::SOME_CONSTANT.

Then the explicit use of mysqli_* inside your class. If you only ever are gonna use mysqli_* that's fine otherwise use the Adapter pattern and pass it the "driver" your wanting to use. At the very least I would create an interface for the SQL_Conn class so that it becomes interchangeable in your code and not as tight as a knot. function doSomething(SQL_ConnInterface $conn) not function doSomething(SQL_Conn $conn), the latter is tightly coupled to the SQL_Conn class while the former allows me to implement the SQL_ConnInterface and provide my own implementation.

Upgrade your skillset, and make your classes follow the PSR standard @ https://github.com/php-fig/fig-standards/tree/master/accepted.

There is a lot of code, and some methods body go way beyond screen height, some refactoring will help to make it maintainable, breaking the class up into some smaller parts with a distinct responsibility may help, for example QueryBuilder has nothing to do with a SQL Connection, you can easily build a query without having a connection to any database, all you need is to know the platform you are building it for.