Throwing exceptions in a database wrapper

Question

I hope the extension methods ThrowArgumentNullExceptionIfNullOrEmpty and ThrowNullReferenceExceptionIfNullOrEmpty are straightforward.

I have a property in my class called ConnectionString. I can set this property through a method called SetConnectionString.

public static void SetConnectionString(string connectionString)
{
        _connectionString = connectionString.ThrowArgumentNullExceptionIfNullOrEmpty("connectionString", Localizer.GetString("0x000000004"));
}

My reason why I don't have

public static string ConnectionString { get; set; }

but

public static string ConnectionString { get { return _connectionString; } }

is because I want the property to be read-only and throw an ArgumentNullException if the argument in SetConnectionString is null or empty.

Since the class is static, it's possible for ConnectionString to be null (or an empty string if I do this: return _connectionString ?? "").

Part 1

In the code below I check the arguments for null or an empty string but to prevent an exception occurring when calling OpenConnection (see Part 2 for more on this method) I also check if the _connectionString (private variable of the ConnectionString property) is not null or empty (empty is for the internal connection string of the OleDbConnection object). Here I decided the exception should be a NullReferenceException because it's not directly an argument of the method.

Is this the correct approach to handle this situation? Consider that the code in between the checks might and might not have an impact on the application if the using fails.

public static int GetHighestID(string table, string column)
{
    _connectionString.ThrowNullReferenceExceptionIfNullOrEmpty(Localizer.GetString("0x000000000"));

    table.ThrowArgumentNullExceptionIfNullOrEmpty("table", Localizer.GetString("0x000000001"));

    column.ThrowArgumentNullExceptionIfNullOrEmpty("column", Localizer.GetString("0x000000002"));

    // Some other things are happening here ...

    using (var connection = new OleDbConnection(_connectionString))
    using (var command = new OleDbCommand(string.Format("SELECT MAX({0}) FROM {1}", column, table), connection))
    {
        OpenConnection(connection);

        return (int)command.ExecuteScalar();
    }
}

Part 2

The OpenConnection method as mentioned before is included below.

public static void OpenConnection(OleDbConnection connection)
{
    connection.ThrowArgumentNullExceptionIfNull("connection", Localizer.GetString("0x00000000C"));
    connection.ConnectionString.ThrowArgumentNullExceptionIfNullOrEmpty("connection.ConnectionString", Localizer.GetString("0x00000000F"));

    if (connection.State != ConnectionState.Open)
        connection.Open();
}

I also here check if the OleDbConnection object is not null and that the internal connection string is not null or an empty string. My intention is to always use this method when opening the connection with the database.

Again, is this the correct approach to handle this situation?

Answer 1

Are you sure you want your database wrapper to be static? What if you want to mock it in your unit tests?

I would make it a plain old class, pass the connection string through the constructor and do any kind of checks that I want right there, because that means your database wrapper won't be allowed to even exist without a valid connection string. With your current design, you're forced to check the connection string in each method, therefore duplicating code, and duplication is generally a no-no.

Static classes do have their use cases, but this is really not one of them. You usually want to write static classes to group methods that don't deal with any specific object, but here you are indeed dealing with something that, being a database, is about as specific as it gets.

Also, what's the GetHighestID method for? I'm asking because I had to deal with codebases that, instead of letting the database do the auto-increment, would exactly do a SELECT MAX(Id) and add 1 before doing INSERTs.

In case that is what you're doing, then stop, because it defeats the purpose of having an auto-increment column at all. Also, the fact that the column is set to auto-increment is something your code shouldn't need to know about. As far as your code is concerned, all it needs to know is the name of the field that is the designated key for the table. Plus, a database can handle auto-increments much better than you ever will, even more so when it comes to concurrent writes.

Last but not least, exceptions. Don't throw NullReferenceExceptions. That's the job of CLR, not yours. Now, with my design the need to throw it just goes away, but I just thought I would mention this. Throwing a NullReferenceException manually stinks.

Edit

Well, the reason why I made it static is because the class has to be used in several classes. I have for ex. classes like Item, Person, Factory, ... which all have an interaction to the database. Each class has an Insert, Update, ... method. In my POV it looks easier to set the connection once e.g. while starting the application and then be able to use the class everywhere without having to initialize it every time.

You're right that it looks easier, but it really is not.

Let's examine the options we have here:

1. a static database wrapper, this would be your design
2. a non-static class
3. a non-static class implementing an interface

Option 2 would look like this:

class ClientClass
{
    private readonly Database _database;

    public ClientClass()
    {
        _database = new Database(ConnectionString);
    }
}

Now, the problem with options 1 and 2 is that, should you ever want to write unit tests for one of the classes that uses your database wrapper without actually issuing queries to the database, you would be out of luck.

And since you should be writing unit tests, you are indeed out of luck.

Why is that, though? Because you can't create a class that looks like your database wrapper but, for instance, returns the data you decide upon calling a hypothetical Select method.

Option 1 doesn't allow that because, even if you wrote such a class, you would have no way of substituting your FakeDatabase class without physically replacing FakeDatabase with Database in all the classes that use it. Static classes are initialized only once, so how are you going to swap the real implementation with a fake one?

Option 2 doesn't allow that either, because the name of your database wrapper is wired in the constructor of the class. There is still no way you can swap Database with FakeDatabasewithout touching the code.

Plus, option 2 is just bad because now your client classes need to know that a connection string exists at all, when they really shouldn't.

Option 3 is described down here:

From what you say I understand that I should initialize the class each time and pass the connection string before calling the method. If not, what approach would you suggest?

That's correct. As @craftworkgames said, classes that need to communicate with the database get an instance of the database wrapper passed in, generally through the constructor.

To be even more precise, once your database wrapper is ready, you will want to extract an interface, let's call it IDatabase, that exposes all the wrapper's public method, and have your wrapper implement it.

Client classes, then, will have a constructor such as this:

class ClientClass
{
    private readonly IDatabase _database;

    public ClientClass(IDatabase database)
    {
         _database = database;
    }
}

Now you're golden, because if FakeDatabase implements IDatabase too, when writing a unit test you can pass in an instance of FakeDatabase with your client class none the wiser.

Client classes only need to know that the class you pass in exposes a certain set of methods. It does and should not know anything about how and from where that class pulls the data it returns, or where it writes to. Is it a RDBMS? Is it an XML file? Something else altogether? Client classes don't have to care about that, which is good, because it gives you the option to swap data stores without changing anything else.

Option 3 is called dependency injection. All the information you need is just a google away.

You could also read about:

1. programming to the interface, not the implementation
2. unit testing
3. mock objects

Lastly, this concept of making classes as unaware of the workings of other classes as possible, applies to every level of your architecture. A database layer just happens to be one of the prime examples.

Gosh, I hope all of that made sense.