5
\$\begingroup\$

I have created a DatabaseConnection class as this:

class DatabaseConnection
{
    public SqlConnection sqlconn { get; set; }
    public DatabaseConnection(string server, string db, string user, string pass)
    {
        sqlconn = new SqlConnection(@"Data Source=" + server + ";Initial Catalog=" + db + ";User ID=" + user + ";Password=" + pass);
        OpenConn();
    }

    public SqlConnection OpenConn()
    {
        try{
            if (sqlconn != null && sqlconn.State == ConnectionState.Closed)
                sqlconn.Open(); // MessageBox.Show("Connection has been successfully established");
        }

        catch(Exception ex){
            MessageBox.Show(ex.Message);
        }

        return sqlconn;
    }
}

Its only purpose is to open the SqlConnection. I am just following the Single Responsibility Principle.

And then, in every class that needs a DatabaseConnection, this is my approach:

class DataViewer
{
    private DatabaseConnection dbCon;
    private SqlConnection sqlconn;
    private SqlDataAdapter sqlDA;

    //Here I am applying constructor injection
    public DataViewer(DatabaseConnection dbCon)
    {
        this.dbCon = dbCon;
        this.sqlconn = dbCon.sqlconn;
    }
    public DataGridView getDataInStoredProc(DataGridView dtgrdView, string storedProcName, params SqlParameter[] parameters)
    {
        try{
            //Some logic here
            sqlconn.Close();
        }

        catch (Exception ex){
            MessageBox.Show(ex.Message);
            sqlconn.Close();
        }
        return dtgrdView;
    }
}

Now, I would like to know if I am doing good or bad approach for this. If not, could you please explain me why and give me some advice on how to do it right?

\$\endgroup\$
  • \$\begingroup\$ I suggest you not accept and answer in the first hour. I don't like the way the connections are managed. \$\endgroup\$ – paparazzo Jan 23 '17 at 10:55
3
\$\begingroup\$

I see a few issues with your class:

Issue 1

This property is open so anyone can set it:

public SqlConnection sqlconn { get; set; }

You probably want to make the setter private so only you can set it from within the class and it can be accessed from outside.

Issue 2

The property name should be SqlCon because the convention is to use Pascal naming in C#.

Issue 3

You have this in your constructor: OpenConn(); which means as soon as someone creates an instance of your class, you open the connection but what if the user of your class does not want to open the connection yet?

Issue 4

Why are you making the assumption that the user of you class will be using windows forms? What if I use your class from a console application, how will MessageBox.Show work?

catch(Exception ex){
    MessageBox.Show(ex.Message);
}

Issue 5

What if the user of your class forgets to close the connection?

Issue 6

What are you getting from this class? I will not use your class because the one in .NET is much better and easier to use. I can use it like this:

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    // Do my work
}

So if I am on your team and you tell me to use your class, which is a wrapper, first question I would ask you is: Why? And then if I find out that as soon as I create an instance of your class, you are opening database connections, I will not be very happy.


Patterns

Patterns such as Single Responsibility Pattern (actually it's a principle but beyond the point) and many other patterns that are there should be used by programmers who have a black belt, or at least a few belts, in programming. If someone is learning programming, they should not even bother with patterns. Patterns are the cool thing and with names like Singleton, Liskov Substitution Principle, Memento Pattern and other fancy names, I have to admit they are so tempting. People throw these terms around because they want to sound smart but do not give into this temptation. People want to use them instead of just creating a plain solid class.

Many people on StackOverflow ask questions such as "Why do people give so much attention to data structures, will I ever need to create one?" My answer to this question is simple: Yes, you will create them every single time you create a class. Just because you are creating a class called DatabaseConnection or Employee does not mean they are not data structures. They are. They are encapsulating data and you better make sure no one puts the state of your class into a bad state: like you have the sqlconn property public.

Forget Patterns for now

Do not worry about patterns for now. Just study data structures and learn how much effort is put into making the data structure so it is never in a bad state. Pay attention to how the operations are defined so users can interact with the data, add data, manipulate data, delete data and what it does when someone tries something they should not be allowed. In other words, when someone tries something the are not allowed does the data structure show a message box? Does it write to the console? Does it write to a file?

At the end of the day, in C# that is and many other OO languages, data structures are just good solid classes. If you know how to make your own ArrayList class, which is an array that can grow and shrink, and you can make it in a way that the class will never be in a bad state, regardless of how irresponsible or clever the programmer who is using your class is, then you will know how to make good solid classes.

Then you can start looking at patterns.

I am sorry if I sound harsh in any way, it is just because I want so speak plainly and I think this is the best way to get the message across.

\$\endgroup\$
  • \$\begingroup\$ I appreciated your answers but I didn't use patterns just to sound smart. I am using this because I want myself to practice and become familiar with this approach as this is one of the most important concept of programming. That's why I made a sample and I knew it wouldn't be perfect at first so I tried to ask someone who has a better knowledge regarding this. :) I'm pretty sure every smart programmer wasn't able to perfect this at first, including you right? Anyway, thank you very much for your answer. It really helps me a lot. Specially when you pointed out the issues. \$\endgroup\$ – Jhe Jan 23 '17 at 5:05
  • \$\begingroup\$ @Jhe another way to say thank you on Stack Exchange sites is to upvote answers that you find helpful. \$\endgroup\$ – t3chb0t Jan 23 '17 at 5:11
  • 1
    \$\begingroup\$ Don't get me wrong. I went through the same thing and no one told me to not worry about patterns and just learn the important fundamentals. Believe me those patterns are all nothing but fancy names once you fully understand them. Concentrate on learning writing good solid classes even if they are not following a pattern. I was not talking about you wanting to sound smart but the community in general. It actually does not apply to you because you are learning. There are programmers out there who write very bad quality code but they can beat anyone in a debate about patterns ;) \$\endgroup\$ – CodingYoshi Jan 23 '17 at 5:13
  • 1
    \$\begingroup\$ @t3chb0t sorry, I can't upvote for now as Stack Exchange says "Vote cast by those with less than 15 reputation are recorded, but do not change the publicly displayed post score." :) \$\endgroup\$ – Jhe Jan 23 '17 at 5:14
  • 2
    \$\begingroup\$ @Jhe one final thought: learn the SOLID principles first. If you know them and can implement them then patterns will emerge as a consequence of applying those principles. You might not know many patterns yet, but if you are able to create SOLID classes and you read about patterns later, it's very likely that you'll find that you've been already using patterns without even knowing it was this or that. \$\endgroup\$ – t3chb0t Jan 23 '17 at 6:11
1
\$\begingroup\$

While the idea is a decent one, the approach has many holes. Let's tart with the DatabaseConnection class. .NET already has an abstract DbConnection class which SqlConnection inherits from and both (obviously) implement the IDbConnection interface. This implementation seems to want to create and manage SqlConnection objects, as well as maintain its state, AND do some error reporting via the UI (MessageBox call). This is itself a violation of the Single Responsibility principle. It does three different things.

So let's move those three things apart and see what we get. First, let's get a thing that creates database connections:

static class DatabaseFactory
{
    public static IDbConnection CreateDbConnection(string server, string db, string user, string pass)
    {
        DbConnectionStringBuilder builder = new SqlConnectionStringBuilder
        {
            DataSource = server,
            InitialCatalog = db,
            UserID = user,
            Password = pass
        };

        return new SqlConnection(builder.ConnectionString);
    }

    public static IDbCommand CreateCommand(string storedProcName, params DbParameter[] parameters)
    {
        var storedProcedure = new SqlCommand(storedProcName)
        {
            CommandType = CommandType.StoredProcedure
        };

        storedProcedure.Parameters.AddRange(parameters);
        return storedProcedure;
    }
}

Note that I used the SqlConnectionStringBuilder class to put together the connection string rather than string concatenation. Eliminates any potential typos and it sanitizes/escapes any strings properly. By the by, I didn't do this here, but parameter names such as pass should really be spelled out to password and the like as (in this case), the shortened version is another word altogether that can be confusing.

Next, something is needed to manage connection state. Let's put that together:

class DatabaseConnectionManager : IDatabaseConnectionManager
{
    private readonly IDbConnection _Connection;

    public DatabaseConnectionManager(IDbConnection connection)
    {
        this._Connection = connection;
    }

    public IDbConnection Connection
    {
        get
        {
            return this._Connection;
        }
    }

    public void Open()
    {
        if ((this._Connection != null) && (this._Connection.State == ConnectionState.Closed))
        {
            this._Connection.Open();
        }
    }

    public void Close()
    {
        if ((this._Connection != null) && (this._Connection.State == ConnectionState.Open))
        {
            this._Connection.Close();
        }
    }
}

This thing is really kinda boring, to be honest. It does things that SqlConnection already does, but that's not the point. You can change the constructor to pass in loggers or what-not to add on to what you need out of the class. But as-is, it does what we want. Three things to note here, unlike the original, this one does not open the connection in the constructor. Reason being, as a best practice, constructors should not do something that may cause an exceptional condition to occur and opening a database connection is certainly an opportunity for that. Second, the try..catch is gone. There's not much the class can do with it -- the original threw up a MessageBox which is a mixing of concerns, but we'll leave someone higher up the call chain to catch it and do something with it (unless you need logging as above). The last thing is that the class implements a new interface, called IDatabaseConnectionManager. Here it is in all its glory:

interface IDatabaseConnectionManager
{
    void Open();

    IDbConnection Connection { get; }

    void Close();
}

Kinda cut and dry, but it will allow us to develop to interfaces.

So that brings us to the DataViewer class. It's another blending of UI and data, which feels weird. Let's keep it data-only for a moment. Let's say it's a class designed to execute a single stored procedure and return some data. I've left out some details, but you can fill those in! :)

class DataViewer : IDataViewer
{
    private IDatabaseConnectionManager _ConnectionManager;

    public DataViewer(IDatabaseConnectionManager connectionManager)
    {
        this._ConnectionManager = connectionManager;
    }

    public DataSet GetDataInStoredProc(IDbCommand storedProcedure)
    {
        storedProcedure.Connection = this._ConnectionManager.Connection;
        this._ConnectionManager.Open();
        try
        {
            // Logic here to get and return data somehow.
        }
        finally
        {
            this._ConnectionManager.Close();
        }
    }
}

Another interface for our use:

interface IDataViewer
{
    DataSet GetDataInStoredProc(IDbCommand storedProcedure);
}

Okay. So let's put it all together somewhere (semi pseudo-code)...

using (var connection = DatabaseFactory.CreateDbConnection("localhost", "products", "test", "p455w0rd"))
{
    var manager = new DatabaseConnectionManager(connection);
    var viewer = new DataViewer(manager);

    using (var procedure = DatabaseFactory.CreateCommand("GetProducts"))
    {
        var products = viewer.GetDataInStoredProc(procedure);

        myDataGrid.DataSource = products;
    }
}

This is all only semi-usable code. You'll want to fill in the blanks appropriately. There's a few new abstractions and layers to keep pieces separated so that they may be replaced with other versions or mock versions for unit testing.

\$\endgroup\$
  • 3
    \$\begingroup\$ No IDisposable? \$\endgroup\$ – 410_Gone Jan 23 '17 at 3:59
  • 1
    \$\begingroup\$ None of these classes both create and store any IDisposable objects. Leaving it to the caller. \$\endgroup\$ – Jesse C. Slicer Jan 23 '17 at 4:07
  • 1
    \$\begingroup\$ Without IDisposable you need a try/catch to manage a connection instead of a much easier to use using statement - this is very unusual for such scenarios. \$\endgroup\$ – t3chb0t Jan 23 '17 at 5:16
  • \$\begingroup\$ Ok, time out.. Look at the code at the bottom. What needs IDisposable that doesn't already use it? There are appropriate usings where needed. \$\endgroup\$ – Jesse C. Slicer Jan 23 '17 at 5:19
  • \$\begingroup\$ var c = new DatabaseConnectionManager(null); c.Open(); Great I will continue using c. \$\endgroup\$ – CodingYoshi Jan 23 '17 at 5:31

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.