patterncsharpMinor
Generic getting single value from DB in C#
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:
One explanation. The purpose of
What do you think about this piece of code?
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
-
C# convention is to use language aliases
-
Also along the lines of convention, the word is "database", not "data base" and therefore the class should not be named
-
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,
-
Class member variables should always be
-
Use
So, here's a cut at that:
Sample calling code:
As a final note, you'll likely want to check for
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.