What’s Wrong With This Code: Class Design

Here is the second post in this series. Proper class/ type design is very important for your app for many reasons. Unfortunately I see issues all the time! Even with supposed senior programmers (like in the example below). So lets see if you can spot all the issues with this class below… there are multiple of them (most of the code has been removed from brevity).

  1. public class DataAccess
  2. {
  3.     #region constants
  4.     //default connection string
  5.     private string CONN_STRING = WebConfigurationManager.ConnectionStrings[“ConnectionString”].ConnectionString;
  6.     private SqlConnection connection;
  7.     #endregion
  8.     public DataAccess()
  9.     {
  10.         connection = new SqlConnection(CONN_STRING);
  11.     }
  12.     private void OpenConnection()
  13.     {
  14.         if (connection.State != System.Data.ConnectionState.Open)
  15.         {
  16.             connection.Open();
  17.         }
  18.     }
  19.     private void CloseConnection()
  20.     {
  21.         if (connection.State != System.Data.ConnectionState.Closed)
  22.         {
  23.             connection.Close();
  24.         }
  25.     }
  26. }

Submit your answer using comments.

6 thoughts on “What’s Wrong With This Code: Class Design

  1. I’d say that First of all, errors that could happen while opening the new connection are not trapped…
    There should be a constructor with a Connection String as parameter for testing purpose.
    Thats all I can see right now, i’ll edit my post if I can find more !

  2. Well lets see….
    1.) There is no way to guarantee a connection gets closed once opened, aka dispose, at the very least if using this pattern a finalizer should be used.
    2.) Connection pooling generally takes care of the need to keep track of a connection so you should just create one when you need it and close it when you re done unless you have a very specific performance metric.
    3.) The connection is created in open and then not checked for null before closing it.
    4.) Multi threading is basically impossible if this is your shared data class which is why i would assume you would make code such as this in the first place.

  3. 1) no constants in the constants region

    2) no comments about what the class is for, what it does.. etc.

    3) no comments for the explicit constructor, or any method. This may be a matter of debate, but there should be /// comments for each method at the least.

    4) hardcoded index for ConnectionStrings collection

    5) Both the OpenConnection() and CloseConnection() methods are private. With that access modifier, no instance of this class can invoke these methods – so unless there is more code in the class, they appear useless.

    6) Lastly, there is no exception handling here, so hopefully the instantiating code would handle those?

    I agree with Cosmo’s first point, but I don’t see his 3rd. The connection is created in the explicit constructor. Maybe I’m wrong, but I would expect that would at least produce a non-null instance of SqlConnection, so you’d have a valid connection object to work with in any instance of this class.

    I truly don’t understand what Jason is talking about on any of his points, so if he’s right, it would be really useful to have an in-depth explanation of what he’s referring to. Why is it a problem if nothing is ‘abstracted’?

    I will say that I have seen code developed by much younger people (than me) that is so abstract it is nearly impossible to follow. Anonymous generic delegates of abstract instances of blah.. blah.. and you can’t find the code that actually /does/ anything. There is a value to re-usable code, but abstraction for the sake of abstraction can be absurd – and absurdly complex. I prefer simple and concrete to abstract and complex.

  4. Mike,

    If you do not know what IDisposable is, or what unit tests are, you should read some more blogs before commenting on any of them.

    As far as abstraction, we are talking about the same thing but from different viewpoints. Anytime you create a class you are abstracting some form of processing which should make it easier to use and re-use. This class appears to wrap a connection but in the process hides important details like the ones you pointed out: exception handling, life cycle, state, configuration, etc.., without making the connection itself any easier to use or re-use.

    I hope this clarifies my previous reply.

Comments are closed.