11

Let's say that we have the following interface -

interface IDatabase { 
    string ConnectionString{get;set;}
    void ExecuteNoQuery(string sql);
    void ExecuteNoQuery(string[] sql);
    //Various other methods all requiring ConnectionString to be set
}

The precondition is that ConnectionString must be set/intialized before any of the methods can be run.

This precondition can be somewhat achieved by passing a connectionString via a constructor if IDatabase were an abstract or concrete class -

abstract class Database { 
    public string ConnectionString{get;set;}
    public Database(string connectionString){ ConnectionString = connectionString;}

    public void ExecuteNoQuery(string sql);
    public void ExecuteNoQuery(string[] sql);
    //Various other methods all requiring ConnectionString to be set
}

Alternatively, we can create connectionString a parameter for each method, but it looks worse than just creating an abstract class -

interface IDatabase { 
    void ExecuteNoQuery(string connectionString, string sql);
    void ExecuteNoQuery(string connectionString, string[] sql);
    //Various other methods all with the connectionString parameter
}

Questions -

  1. Is there a way to specify this precondition within the interface itself? It is a valid "contract" so I'm wondering if there is a language feature or pattern for this (the abstract class solution is more of a hack imo besides the need of creating two types - an interface and an abstract class - every time this is needed)
  2. This is more of a theoretical curiosity - Does this precondition actually fall into the definition of a precondition as in the context of LSP?
  • 2
    By "LSP" you guys are talking about Liskov substitution principle? The "if its quack like a duck but needs batteries its not a duck" principle? Because as I see it its more a violation of the ISP and SRP maybe even the OCP but not really the LSP. – Sebastien Jan 17 '17 at 15:37
  • 2
    Just so you know, this whole concept of "ConnectionString must be set/intialized before any of the methods can be run" is an example of temporal coupling blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling and should be avoided, if possible. – Richiban Jan 17 '17 at 16:12
  • Seemann is really a big fan of Abstract Factory. – Adrian Iftode Jan 24 '17 at 8:49
10
  1. Yes. From .Net 4.0 upward, Microsoft provides Code Contracts. These can be used to define preconditions in the form Contract.Requires( ConnectionString != null );. However, to make this work for an interface, you will still need a helper class IDatabaseContract, which becomes attached to IDatabase, and the precondition needs to be defined for every individual method of your interface where it shall hold. See here for an extensive example for interfaces.

  2. Yes, the LSP deals with both syntactical and semantical parts of a contract.

  • I didn't think you could use Code Contracts in an interface. The example you provides shows them being used in classes. The classes do conform to an interface, but the interface itself contains no Code Contract information (a shame, really. That would be the ideal place to put it). – Robert Harvey Jan 18 '17 at 16:10
  • 1
    @RobertHarvey: yes, you are right. Technically, you need a second class, of course, but once defined, the contract works automatically for every implementation of the interface. – Doc Brown Jan 18 '17 at 16:31
21

Connecting and querying are two separate concerns. As such, they should have two separate interfaces.

interface IDatabaseConnection
{
    IDatabase Connect(string connectionString);
}

interface IDatabase
{
    public void ExecuteNoQuery(string sql);
    public void ExecuteNoQuery(string[] sql);
}

This both ensures that IDatabase will be connected when used and makes client not depend on interface it doesn't need.

  • Could be more explicit about "this is a pattern of enforcing the preconditions through types" – Caleth Jan 17 '17 at 10:04
  • @Caleth: this is not a "general pattern of enforcing preconditions". This is a solution for this specific requirement of ensuring the connect happens before anything else. Other preconditions will need different solutions (like the one I mentioned in my answer). I would like to add for this requirement, I would clearly prefer Euphoric's suggestion over mine, because it is much simpler and does not need any additional third party component. – Doc Brown Jan 17 '17 at 10:08
  • The specific requrement that something happens before something else is widely applicable. I also think your answer better fits this question, but this answer can be improved – Caleth Jan 17 '17 at 10:12
  • 1
    This answer completely misses the point. The IDatabase interface defines an object capable of establishing a connection to a database and then executing arbitrary queries. It is the object that acts as the boundary between the database and the rest of the code. As such, this object has to maintain state (such as a transaction) that can affect the behavior of the the queries. Putting them in the same class is very practical. – jpmc26 Jan 17 '17 at 15:06
  • 4
    @jpmc26 None of your objections make sense, as the state can be maintained within class implementing IDatabase. It can also reference the parent class that created it, thus getting access to whole database state. – Euphoric Jan 17 '17 at 15:43
5

Let's take a step back and look at the bigger picture here.

What is IDatabase's responsibility?

It has a few different operations:

  • Parse a connection string
  • Open a connection with a database (an external system)
  • Send messages to the database; the messages command the database to alter it's state
  • Receive responses from the database and transform them into a format the caller can use
  • Close the connection

Looking at this list, you might be thinking, "Doesn't this violate SRP?" But I don't think it does. All of the operations are part of a single, cohesive concept: manage a stateful connection to the database (an external system). It establishes the connection, it keeps track of the current state of the connection (in relation to operations done on other connections, in particular), it signals when to commit the connection's current state, etc. In this sense, it acts as an API that hides a lot of implementation details most callers won't care about. For example, does it use HTTP, sockets, pipes, custom TCP, HTTPS? Calling code doesn't care; it just wants to send messages and get responses. This is a good example of encapsulation.

Are we sure? Couldn't we split up some of these operations? Maybe, but there's no benefit. If you try to split them up, you're still going to need a central object that keeps the connection open and/or manages what the current state is. All the other operations are strongly coupled to the same state, and if you try to separate them, they're just going to end up delegating back to the connection object anyway. These operations are naturally and logically coupled to the state, and there's no way to separate them. Decoupling is great when we can do it, but in this case, we actually can't. At least not without a very different, stateless protocol to talk to the DB, and that would actually make very important problems like ACID compliance much harder. Also, in the process of trying to decouple these operations from the connection, you'll be forced to expose details about the protocol that callers don't care about, since you'll need a way of sending some kind of "arbitrary" message to the database.

Note that the fact we're dealing with a stateful protocol pretty solidly rules out your last alternative (passing connection string as a parameter).

Do we really need connection string to be set?

Yes. You can't open the connection until you have a connection string, and you can't do anything with the protocol until you open the connection. So it's pointless to have a connection object without one.

How do we solve the problem of requiring the connection string?

The problem we're trying to solve is that we want the object to be in a usable state at all times. What kind of entity is used to manage state in OO languages? Objects, not interfaces. Interfaces don't have state to manage. Because the problem you're trying to solve is a state management problem, an interface isn't really appropriate here. An abstract class is much more natural. So use an abstract class with a constructor.

You might also want to consider actually opening the connection during the constructor as well, since the connection is also useless before it's opened. That would require an abstract protected Open method since the process of opening a connection may be database specific. It would also be a good idea to make the ConnectionString property read only in this case, since changing the connection string after the connection is open would be meaningless. (Honestly, I'd make it read only anyway. If you want a connection with a different string, make another object.)

Do we need an interface at all?

An interface that specifies the available messages you can send over the connection and the types of responses you can get back could be useful. This would allow us to write code that executes these operations but isn't coupled to the logic of opening a connection. But that's the point: managing the connection is not part of the interface of, "What messages can I send and what messages can I get back to/from the database?", so the connection string shouldn't even be a part of that interface.

If we go this route, our code might look something like this:

interface IDatabase {
    void ExecuteNoQuery(string sql);
    void ExecuteNoQuery(string[] sql);
    //Various other methods all requiring ConnectionString to be set
}

abstract class ConnectionStringDatabase : IDatabase { 

    public string ConnectionString { get; }

    public Database(string connectionString) {
        this.ConnectionString = connectionString;
        this.Open();
    }

    protected abstract void Open();

    public abstract void ExecuteNoQuery(string sql);
    public abstract void ExecuteNoQuery(string[] sql);
    //Various other methods all requiring ConnectionString to be set
}
  • Would appreciate if the downvoter would explain their reason for disagreeing. – jpmc26 Jan 17 '17 at 18:25
  • Agreed, re: downvoter. This is the correct solution. The connection string should be provided in the constructor to the concrete/abstract class. The messy business of opening/closing a connection is not a concern of the code using this object, and should remain internal to the class itself. I would argue that the Open method should be private and you should expose a protected Connection property that creates the connection and connects. Or expose a protected OpenConnection method. – Greg Burghardt Jan 18 '17 at 13:55
  • This solution is quite elegant and very well design. But I think that some of the reasoning behind the design decisions is wrong. Mainly in the first few paragraphs about the SRP. It does violate the SRP even as explained in "What is IDatabase's responsibility?". Responsibilities as seen for the SRP are not just things a class does or manages. Its also "actors" or "reasons to change". And I think that it does violate the SRP because "Receive responses from the database and transform them into a format the caller can use" has very different reason to change than "Parse a connection string". – Sebastien Jan 18 '17 at 15:28
  • Still I upvote this. – Sebastien Jan 18 '17 at 15:29
  • 1
    And BTW, SOLID are not the gospel. Sure they are very important to keep in mind when designing a solution. But you CAN violate them if you know WHY you do it, HOW it is going to affect your solution and HOW to fix things with refactoring if it does gets you into trouble. Thus I think even if the above mentioned solution violates the SRP it is the best one yet. – Sebastien Jan 18 '17 at 15:33
0

I really don't see the reason to have an interface at all here. Your database class is SQL-specific, and really just gives you a convenient/safe way to make sure you aren't querying on a connection that isn't opened properly. If you insist on an interface though, here's how I'd do it.

public interface IDatabase : IDisposable
{
    string ConnectionString { get; }
    void ExecuteNoQuery(string sql);
    void ExecuteNoQuery(string[] sql);
    //Various other methods all requiring ConnectionString to be set
}

public class SqlDatabase : IDatabase
{
    public string ConnectionString { get; }
    SqlConnection sqlConnection;
    SqlTransaction sqlTransaction; // optional

    public SqlDatabase(string connectionStr)
    {
        if (String.IsNullOrEmpty(connectionStr)) throw new ArgumentException("connectionStr empty");
        ConnectionString = connectionStr;
        instantiateSqlProps();
    }

    private void instantiateSqlProps()
    {
        sqlConnection.Open();
        sqlTransaction = sqlConnection.BeginTransaction();
    }

    public void ExecuteNoQuery(string sql) { /*run query*/ }
    public void ExecuteNoQuery(string[] sql) { /*run query*/ }

    public void Dispose()
    {
        sqlTransaction.Commit();
        sqlConnection.Dispose();
    }

    public void Commit()
    {
        Dispose();
        instantiateSqlProps();
    }
}

The usage might look like this:

using (IDatabase dbase = new SqlDatabase("Data Source = servername; Initial Catalog = MyDb; Integrated Security = True"))
{
    dbase.ExecuteNoQuery("delete from dbo.Invoices");
    dbase.ExecuteNoQuery("delete from dbo.Customers");
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.