HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Generic getting single value from DB in C#

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
genericgettingvaluesinglefrom

Problem

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(string query)
        where T : IComparable, IConvertible, IEquatable
    {
        Object value = GetValue(query);
        if (Convert.IsDBNull(value))
            return GetDefaultValue();
        return (T)Convert.ChangeType(value, typeof(T));
    }

    public static T GetDefaultValue()
        where T : IComparable, IConvertible, IEquatable
    {
        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 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?

Solution

-
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?

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

-
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.

-
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.

-
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.

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

-
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(string query)
        where T : IComparable, IConvertible, IEquatable
    {
        var value = this.GetValue(query);

        return Convert.IsDBNull(value) ? GetDefaultValue() : (T)Convert.ChangeType(value, typeof(T));
    }

    public static T GetDefaultValue()
        where T : IComparable, IConvertible, IEquatable
    {
        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.

Code Snippets

public interface IDatabaseAdapter
{
    IDbConnection GetConnection();

    IDbCommand GetCommand(IDbConnection connection, string query);
}
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;
    }
}
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));
    }
}
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()
    {

    }
}

Context

StackExchange Code Review Q#95956, answer score: 5

Revisions (0)

No revisions yet.