current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm looking for any comments or feedback on my database access class. Security and speed are two things I'm most concerned about. One thing to note is this class has to work in a c# .net 2 environment so anything thats more modern would be interesting to me, but please note in the title of your answer if the comments/feedback require newer .net version.

using Microsoft.Practices.EnterpriseLibrary.Common;
using Microsoft.Practices.EnterpriseLibrary.Data;
using System.Data;
using System.Data.SqlClient;
using System.Data.Common;

/// <summary>
/// This is the base class for database access classes. This is the only
/// class that should directly talk to the database. Every class or page
/// that neads to access the database should be refering to this or a
/// derived class.
/// </summary>
public class DatabaseAccess
{
    static string LastDatabaseName = "";
    static Database database = null;
    static int errorCount = 0;

    /// <summary>
    /// Execute a SQL statement on the default database
    /// </summary>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string SQL)
    {
        List<SqlParameter> Parameters = new List<SqlParameter>();
        return ExecSQL("", SQL, Parameters);
    }
    /// <summary>
    /// Execute a SQL statement on the default database
    /// </summary>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <param name="Parameters">The parameters for the SQL statement</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string SQL, List<SqlParameter> Parameters)
    {
        return ExecSQL("", SQL, Parameters);
    }
    /// <summary>
    /// Execute a SQL statement on the requested database
    /// </summary>
    /// <param name="DatabaseName">The database to execute the SQL on</param>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string DatabaseName, string SQL)
    {
        List<SqlParameter> Parameters = new List<SqlParameter>();
        return ExecSQL(DatabaseName, SQL, Parameters);
    }
    /// <summary>
    /// Execute a SQL statement on the requested database
    /// </summary>
    /// <param name="DatabaseName">The database to execute the SQL on</param>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <param name="Parameters">The parameters for the SQL statement</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string DatabaseName, string SQL, List<SqlParameter> Parameters)
    {
        // Database access variables
        // Database database = null;
        DbCommand command = null;
        DataTable table = new DataTable();

        if (DatabaseName != LastDatabaseName || database == null)
        {
            if (database != null)
                database = null;

            if (DatabaseName != "")
                database = DatabaseFactory.CreateDatabase(DatabaseName);
            else
                database = DatabaseFactory.CreateDatabase();
        }
        LastDatabaseName = DatabaseName;

        command = database.GetSqlStringCommand(SQL);
        foreach (SqlParameter p in Parameters)
        {
            database.AddInParameter(command, p.ParameterName, p.DbType, p.Value);
        }

        try
        {
            if (!SQL.StartsWith("UPDATE") && !SQL.StartsWith("DELETE"))
                table = database.ExecuteDataSet(command).Tables[0];
            else
                database.ExecuteNonQuery(command);

            errorCount = 0;
        }
        catch (SystemException e)
        {
            errorCount++;

            if (errorCount < 2)
            {
                CMSLog.Exception(e);
                CMSLog.Info(SQL);
                CMSUtil.setSession("Exception", e.Message);
                CMSUtil.setSession("ExceptionExtra", e.StackTrace);
                HttpContext.Current.Response.Redirect("~/CMS/SiteError.aspx");
            }
            else
            {
                HttpContext.Current.AddError(new Exception("Looping DB Error: " + e.Message));
            }
        }

        return table;
    }
}

A simple example using the class:

string strValue = "Some Untrusted Value";
List<SqlParameter> parms = new List<SqlParameter>();
parms.Add(new SqlParameter("Value", strValue));

string sql = "SELECT * FROM TableName WHERE FieldName=@Value";
DataTable tblResults = DatabaseAccess.ExecSQL(sql, parms);

Updated Class

Spelling fixed, Using string.Empty, Using IEnumerable, Remove unneeded local variables, Marked class variables private

/// <summary>
/// This is the base class for database access classes. This is the only
/// class that should directly talk to the database. Every class or page
/// that needs to access the database should be referring to this or a
/// derived class.
/// </summary>
public class DatabaseAccess
{
    private static string LastDatabaseName = string.Empty;
    private static Database database = null;
    private static int errorCount = 0;

    /// <summary>
    /// Execute a SQL statement on the default database
    /// </summary>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string SQL)
    {
        return ExecSQL(string.Empty, SQL, new List<SqlParameter>());
    }
    /// <summary>
    /// Execute a SQL statement on the default database
    /// </summary>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <param name="Parameters">The parameters for the SQL statement</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string SQL, IEnumerable<SqlParameter> Parameters)
    {
        return ExecSQL(string.Empty, SQL, Parameters);
    }
    /// <summary>
    /// Execute a SQL statement on the requested database
    /// </summary>
    /// <param name="DatabaseName">The database to execute the SQL on</param>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string DatabaseName, string SQL)
    {
        return ExecSQL(DatabaseName, SQL, new List<SqlParameter>());
    }
    /// <summary>
    /// Execute a SQL statement on the requested database
    /// </summary>
    /// <param name="DatabaseName">The database to execute the SQL on</param>
    /// <param name="SQL">The SQL statement to execute</param>
    /// <param name="Parameters">The parameters for the SQL statement</param>
    /// <returns>DataTable of selected results</returns>
    public static DataTable ExecSQL(string DatabaseName, string SQL, IEnumerable<SqlParameter> Parameters)
    {
        // Database access variables
        DbCommand command = null;
        DataTable table = new DataTable();

        // Make a connection to the database if needed
        if (DatabaseName != LastDatabaseName || database == null)
        {
            // Force open connections to close
            database = null;

            if (DatabaseName != string.Empty)
                database = DatabaseFactory.CreateDatabase(DatabaseName);
            else
                database = DatabaseFactory.CreateDatabase();
        }
        LastDatabaseName = DatabaseName;

        // Create the database command and add the passed parameters
        command = database.GetSqlStringCommand(SQL);
        foreach (SqlParameter p in Parameters)
        {
            database.AddInParameter(command, p.ParameterName, p.DbType, p.Value);
        }

        try
        {
            // Run the SQL
            if (!SQL.StartsWith("UPDATE") && !SQL.StartsWith("DELETE"))
                table = database.ExecuteDataSet(command).Tables[0];
            else
                database.ExecuteNonQuery(command);

            errorCount = 0;
        }
        catch (SystemException e)
        {
            errorCount++;

            if (errorCount < 2)
            {
                // Log any errors to the database
                CMSLog.Exception(e);
                CMSLog.Info(SQL);
                CMSUtil.setSession("Exception", e.Message);
                CMSUtil.setSession("ExceptionExtra", e.StackTrace);
                HttpContext.Current.Response.Redirect("~/CMS/SiteError.aspx");
            }
            else
            {
                // Cought a recursive error, break out.
                HttpContext.Current.AddError(new Exception("Looping DB Error: " + e.Message));
            }
        }

        return table;
    }
}
share|improve this question
 
In your summary, "neads" should be "needs" and "refering" should be "referring". ;p –  Steven Jeuris May 24 '11 at 9:40
add comment

2 Answers

up vote 3 down vote accepted

A few quick ideas:

  • Check spelling.
  • Use string.Empty instead of "" for improved readability and performance.
  • Always use visibility modifiers - For example your fields lack the typical "private" keyword. Example private static Database database = null;
  • Re-evaluate your design choice to go with a static class. Static classes are know for causing head-aches such as threading problems. Read more here to start with if you are unsure. Just removing all "static" keywords will make the class just as usable.
  • Use lower case for local variables and parameters. For example: "var parameters = new List();"
  • As for the error-counting-logic I don't even know where to start... :-/ Perhaps the whole thing can be done in some other way.
  • Consider the naming of ExecSQL - SQL commands can be both inserts and selects and also other types of commands, while this class concerns itself with select
  • Consider using IEnumerable<T> instead of List<T> since you're only iterating the List<T>.

If you use c# 3.0 or later

  • Use the var keyword if target type is redundant. Example List<SqlParameter> Parameters = new List<SqlParameter>();
share|improve this answer
1  
var is part of C# 3.0 –  Snowbear May 24 '11 at 15:47
 
Hi and thanks. To explain the error counting logic (I'll add comments to the code as well). The CMSLog class logs information into the database and itself uses the DatabaseAccess class. The counting catches errors network/connectivity errors, which would result in an endless loop, and breaks the loop with an error page. The idea is all possible errors are logged without stopping the application. –  Justin808 May 24 '11 at 16:51
 
@Justin808 np. yeah I got it. It is just... done in a very strange way for me. –  vidstige May 24 '11 at 19:10
 
@Snowbear yeah, thanks. Edited the post now... –  vidstige May 24 '11 at 19:10
add comment
up vote 1 down vote

1) 

    if (DatabaseName != LastDatabaseName || database == null)
    {
        if (database != null)
            database = null;

        if (DatabaseName != "")
            database = DatabaseFactory.CreateDatabase(DatabaseName);
        else
            database = DatabaseFactory.CreateDatabase();
    }

First two lines inside your if do not make any sense. Anyway you're assigning another value to database variable below. I would write it as: 

    if (DatabaseName != LastDatabaseName || database == null)
    {
        database = DatabaseName != "" ?
                   DatabaseFactory.CreateDatabase(DatabaseName) :
                   DatabaseFactory.CreateDatabase();
    }

2) Define your variables closer to the first assignment place. command variable is defined 10 lines of code before it is assigned and also has some value which is not used at all.

3) Looks like you're following One return rule. I personally do not think this rule should be followed (at least in C#). For example you're assigning dataTable variable in case of select and do nothing more with, only returning it. But I (as maintainer of your code) see you're assigning it and I have to read the method till the end. Just return dataTable right there - this will let me know that nothing done with it later - it will save my time.

4) In case of insert or update statements you're returning empty dataTable. I would return either null to distinguish it from select statement or dataTable with one cell which will contain number of updated entries (at the moment you're swallowing this information).

5) The entire code is not that intuitive which means that it is not easy readable which means that it is not easy maintainable. I would never guess that in order to add parameter to the command I should look for a method in database class.

6) I would never expect that such method will create databases. This is not intuitive at all.

share|improve this answer
 
JIM-compiler - Thanks for the answer. The 'if (database != null) { database = null; }` bit will close an exiting connection if its open and clear out the variable so a new connection can be opened. It prevents hanging connections to the database. –  Justin808 May 24 '11 at 16:54
 
The class will never create a database unless you pass the SQL to ExecSQL to do so. The DatabaseFactory.CreateDatabase method is part of Microsoft.Practices.EnterpriseLibrary and creates a database object to use based on a named connection string in the web.config. Not passing a name to the method uses the defined default connection string in web.config. –  Justin808 May 24 '11 at 16:56
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.