Generic getting single value from DB in C#

Question

Few months ago I posted my code Getting a single value from the DB. I implemented suggested changes and this is how it looks like right now:

public class DataBase : Page
{
 protected static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);
 protected static string ConnectionString;
 public DataBase()
 {
 ConnectionString = GetConnectionString();
 }
 public static String GetConnectionString()
 {
 return ConfigurationManager.ConnectionStrings["abc"].ConnectionString;
 }
 public static T GetValue<T>(string query)
 where T : IComparable, IConvertible, IEquatable<T>
 {
 Object value = GetValue(query);
 if (Convert.IsDBNull(value))
 return GetDefaultValue<T>();
 return (T)Convert.ChangeType(value, typeof(T));
 }
 public static T GetDefaultValue<T>()
 where T : IComparable, IConvertible, IEquatable<T>
 {
 if (typeof(T) == typeof(String))
 return (T)(object)String.Empty;
 return default(T);
 }
private static Object GetValue(string query)
 {
 try
 {
 using (SqlConnection connection = new SqlConnection(ConnectionString))
 using (SqlCommand command = new SqlCommand(query, connection))
 {
 connection.Open();
 return command.ExecuteScalar();
 }
 }
 catch (Exception e)
 {
 LogQueryError(query, e);
 return DBNull.Value;
 }
 }
protected static void LogQueryError(string query, Exception e)
 {
 log.Error(string.Format("Error while executing Query ({0}): {1}", query, e.Message));
 }
}

One explanation. The purpose of where T : IComparable, IConvertible, IEquatable<T> is to have single method for value types and strings. (inspired by C# Generic constraints to include value types AND strings

What do you think about this piece of code?

• RobH: Can I just double check you aren't going to do this: DataBase.GetValue<int>("select someNumber from dbo.parts where partName = '" + someVariable + "'");
• Der Kommissar: @RobH Perhaps the OP should accept a SqlCommand object instead?
• Piotr Nawrot: This is my plan to pass SQL Command as parameter, but later. Right now I've a lot of dynamically generated queries. I want to refactor them one by one to SqlCommand and then use polymorphism to get final effect.
• Der Kommissar: @PiotrNawrot Then perhaps you should look at the suggestion I offered, which allows you to use both at the same time. Once you are ready to make the full transition, you can simply remove the method that takes a string.
• Piotr Nawrot: @EBrown I'll look for sure. This is what I wanted to do anyway, nevertheless you made a few interesting points. Thank you for your time :-)

Answer 1

1. Something smells hinky having this class inherit from something called Page. Like it's mixing UI and data concerns where it shouldn't. What does Page give this class?

2. C# convention is to use language aliases object and string over the CLR types Object and String, respectively.

3. Also along the lines of convention, the word is "database", not "data base" and therefore the class should not be named DataBase, but rather Database.

4. You have two particular dependencies in this class: a connection string and a logger. I'd recommend inverting those dependencies and injecting them into the class at time of construction.

5. Better yet, there are other dependencies, SqlConnection and SqlCommand being created in the GetValue method. These may best be refactored into another class injected into this one.

6. Class member variables should always be private. If you need them exposed to the outside, or subclasses, use properties to control access.

7. Use var where possible.

So, here's a cut at that:

IDatabaseAdapter interface:

public interface IDatabaseAdapter
{
 IDbConnection GetConnection();
 IDbCommand GetCommand(IDbConnection connection, string query);
}

DatabaseAdapter implementation:

public class DatabaseAdapter : IDatabaseAdapter
{
 private readonly string _ConnectionString;
 public DatabaseAdapter(string connectionString)
 {
 this._ConnectionString = connectionString;
 }
 public IDbConnection GetConnection()
 {
 return new SqlConnection(this._ConnectionString);
 }
 public IDbCommand GetCommand(IDbConnection connection, string query)
 {
 var command = new SqlCommand(query, connection as SqlConnection);
 connection.Open();
 return command;
 }
}

Database class:

public class Database
{
 private readonly ILog _Log;
 private readonly IDatabaseAdapter _DatabaseAdapter;
 public Database(ILog log, IDatabaseAdapter databaseAdapter)
 {
 this._Log = log;
 this._DatabaseAdapter = databaseAdapter;
 }
 public string ConnectionString
 {
 get
 {
 return this.ConnectionString;
 }
 }
 protected ILog Log
 {
 get
 {
 return this._Log;
 }
 }
 public T GetValue<T>(string query)
 where T : IComparable, IConvertible, IEquatable<T>
 {
 var value = this.GetValue(query);
 return Convert.IsDBNull(value) ? GetDefaultValue<T>() : (T)Convert.ChangeType(value, typeof(T));
 }
 public static T GetDefaultValue<T>()
 where T : IComparable, IConvertible, IEquatable<T>
 {
 return typeof(T) == typeof(string) ? (T)(object)string.Empty : default(T);
 }
 private object GetValue(string query)
 {
 try
 {
 using (var connection = this._DatabaseAdapter.GetConnection())
 using (var command = this._DatabaseAdapter.GetCommand(connection, query))
 {
 return command.ExecuteScalar();
 }
 }
 catch (Exception e)
 {
 this.LogQueryError(query, e);
 return DBNull.Value;
 }
 }
 protected void LogQueryError(string query, Exception e)
 {
 this._Log.Error(string.Format("Error while executing Query ({0}): {1}", query, e.Message));
 }
}

Sample calling code:

internal static class Program
{
 private static readonly DataBase _Database = new DataBase(
 LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType),
 new DatabaseAdapter(ConfigurationManager.ConnectionStrings["abc"].ConnectionString));
 private static void Main()
 {
 }
}

As a final note, you'll likely want to check for null or empty strings in constructors and raise appropriate exceptions then to keep the state of the objects well-known.

• Piotr Nawrot: I am so grateful for your review. It showed me how much i didn't notice. You get the point, that Page didn't give anything to Database. I found out that Database was inherited by few other classes which needed Page and Database. Thanks to your question now they inherits directly from Page and Database is injected or used statically. I have a question. Do you think that additional static constructor of Database with default ILog and IDatabaseAdapter is a good idea? It is very convenient to use all this methods as static.
• Piotr Nawrot: I have one more question. Why did you make LogQueryError() and GetValue() non-static?
• Jesse C. Slicer: No, I do not think having a static constructor with default ILog and IDatabaseAdapter is a good idea because it causes tight coupling with the implementation, rather than coding to the interface. Also, those methods are now non-static because they rely on instance-specific member data.
• Piotr Nawrot: Thanks, I get it. Another thing - Why do you open connection in GetCommand()? Shouldn't connection be opened as late as possible? What is more important if you execute .Open() on opened connection you get InvalidOperationException. Am I missing sth?
• Jesse C. Slicer: it's opened in the same place you do in your original code.

Answer 2

Usage of String Queries/Mandatory SQL Injection Bits

One of the things I noticed immediately is the use of string queries.

This may not be a big deal, but it leaves you open to SQL injection. (I know, mandatory SQL injection statement.) An easy way to fix this would be to rewrite a bit of your code to accept a SqlCommand object instead. Perhaps you should optionally allow the user to add a SqlConnection to the SqlCommand on their own as well? In a situation like this, I would never actually use the class you have provided as I have no ability to specify my own SqlConnection or SqlCommand.

(Yes, I know you can change the ConnectionString, but if I want to connect to three different DB's for a set of queries, I would need to create three instances of this DataBase class, and then provide a different ConnectionString for each. Likewise, if I had fifty queries to run in one batch, I would have to suffer the performance of it opening and closing fifty connections. And that's just unacceptable in my opnion. Just as well, the lack of a SqlCommand paramter means I could not provide my own SqlParameter list to do parameterized queries, which would mean for me I could never do a WHERE clause, as I cannot add parameters.)

You could easily do this with something like the following:

public static T GetValue<T>(Command query)
 where T : IComparable, IConvertible, IEquatable<T>
{
 Object value = GetValue(query);
 if (Convert.IsDBNull(value))
 return GetDefaultValue<T>();
 return (T)Convert.ChangeType(value, typeof(T));
}
public static T GetDefaultValue<T>()
 where T : IComparable, IConvertible, IEquatable<T>
{
 if (typeof(T) == typeof(String))
 return (T)(object)String.Empty;
 return default(T);
}
private static Object GetValue(Command query)
{
 try
 {
 if (query.Connection == null)
 using (SqlConnection connection = new SqlConnection(ConnectionString))
 {
 connection.Open();
 return command.ExecuteScalar();
 }
 else
 return command.ExecuteScalar();
 }
 catch (Exception e)
 {
 LogQueryError(query, e);
 return DBNull.Value;
 }
}

This gives you expandability, with the maintainability and SQL injection protection at the client-level. (I assume the users of this class would be quite unhappy if you did nothing to help them protect against SQL injection attacks.)

You could always, for backwards compatibility, leave the GetValue<T>(string query) method in, and rewrite it just a tad:

public static T GetValue<T>(string query)
 where T : IComparable, IConvertible, IEquatable<T>
{
 Object value;
 using (SqlCommand command = new SqlCommand(query))
 value = GetValue(command);
 if (Convert.IsDBNull(value))
 return GetDefaultValue<T>();
 return (T)Convert.ChangeType(value, typeof(T));
}

Portability, maintainability, and it's dynamic enough to handle most any situation.

Lastly, for completeness, this would require another rewrite of the LogQueryError method:

protected static void LogQueryError(SqlCommand query, Exception e)
{
 log.Error(string.Format("Error while executing Query ({0}): {1}", query.CommandText, e.Message));
}

Additional Notes

As Malachi said, other things I would look at changing are the class name (DataBase is awfully broad), and the ConnectionString handling.

If you choose to implement both a method that takes a SqlCommand, and a method that takes a string (for backwards compatibility), you can remove the method that takes a string when you are ready to make a full transition to the SqlCommand method.

• Piotr Nawrot: You are right that with this class in state as posted WHERE statements are possible only when emended with values in query strings. This bad practice, but it was done this way so far. I'm about writing "SqlCommand builder" which will allow custom connection strings and passing parameters I only had to come up with the name for it because SqlCommandBuilder is already present in the framework :-)

Answer 3

Do you want the entire application to have access to ConnectionString? I would think that you want to set how the connection string is changed. You should probably use a private property with a public get/set so that nothing else inadvertently changes the connection string of your Database object in a way you aren't expecting.

Database is a little broad for a class that only does a simple query. I think that you could be more specific with this class's name.

• Piotr Nawrot: There are other methods in this class for executing queries, getting and setting data but I excluded them from post for clarity. Do you still think this class has too broad name? Do you have any suggestion of better one?
• Malachi: I don't know if the Class has too broad of a name, because I don't know what all it does. I don't know who it is, you have hid its personalities from me, I don't know how it will react to being named differently, I don't know how it will react if we change the protected to a private.