How should I structure an application that uses inline SQL to generate data from a database?

Question

I currently have a Data Access Layer that is also exposing some API using a web service.

API Call

[WebMethod]
public List<GlobalStat> GetStats()
{
    List<GlobalStat> Stats = new List<GlobalStat>();

    string sql = @"
       A huge multi-line SQL query
    ";

    try
    {
        string ConString = Constants.connString;
        con = new SqlConnection(ConString);

        cmd = new SqlCommand(sql, con);
        con.Open();
        dr = cmd.ExecuteReader();

        while (dr.Read())
        {
            GlobalStat stat = new GlobalStat();
            stat.Key = dr[0].ToString();
            stat.Value = int.Parse(dr[1].ToString());

            Stats.Add(stat);
        }

    }
    catch (Exception x)
    {

        Response.Write(x);
    }

    return Stats;
}

I am a bit worried about the how the SQL is written.

There are so many things hard-coded into this: Database name, Table names etc.

To solve this problem, do I just create a separate global file with all SQL commands at one place or is there a better paradigm? I am not creating the SQL tables inside the application but these tables reside on a different pre-built database.

How should I structure an application that uses inline SQL to generate data from a database?

Answer 1

The proper way, IMHO, is to call stored procedures. In your C# code you just reference a stored procedure and pass properly typed parameters. Ad hoc SQL built up in your C# code opens the door up for many things - not the least of which are SQL injection and inefficient use of plan cache. Plus it makes it very difficult for you to refactor your queries without recompiling and redeploying the application. That will be necessary for some changes (e.g. when the interface to a stored procedure changes), but shouldn't be necessary for many other typical query changes.

Answer 2

You bring up several issues, and the code you are showing us brings up some more. Things you may want to think about:

• Try to keep the database centric activities constrained into their own classes. No one else needs to know about how to return a list of rows for the GlobalStats object other than the actual class that handles pulling that data out of the database and making it into an actual object for you. No one. If anyone else does know, then your class is not using information hiding (which since we're in an object oriented language, you should be).

• If the object implements IDisposable, then you should be wrapping it with a try {} finally {} block, or even better, wrap it with a using statement (see my second example below).

• Your connection string should only be accessible to the classes that actually need it (part of separating your concerns). Perhaps having a base DataAccess class that has that information?

  public abstract class DataAccess 
  {
      protected const string ConnectionString = "YourConnectionStringHere";
  }
  

Then your repositories can inherit from this class, and you don't have a global static constant that causes your code to be unnecessarily coupled.

• The way your code is right now (if indeed all your code is an example of what you're showing us), then you've probably got an SQL Injection problem. That's worth getting fixed immediately (and I'll show you an example of it below).

  1. Use Parameratized Queries (if you truly must have your SQL in C#).
  2. Separate your concerns (since when does the web API call need to be concerned with a database?
  3. Don't catch exceptions you can't handle.

Here's how I'd write what you're writing (be careful, this code isn't really meant to be used, it's for illustrative purposes only):

[WebMethod]
public List<GlobalStat> GetStats()
{
    GlobalStatsRepository repository = new GlobalStatsRepository();
    List<GlobalStat> stats = repository.GetStats();
    return stats;
}

DataAccessLayer

public class GlobalStatsRepository
{
    public List<GlobalStat> GetStats()
    {

         string sql = @"SELECT * from GlobalStats"; //no, not a good practice

        var stats = new List<GlobalStat>();

        try
        {
            string ConString = Constants.connString;
            conn = new SqlConnection(ConString);

            cmd = new SqlCommand(sql, conn);
            conn.Open();
            dr = cmd.ExecuteReader();

            while (dr.Read())
            {
                GlobalStat stat = new GlobalStat();
                stat.Key = dr[0].ToString();
                stat.Value = int.Parse(dr[1].ToString());

                stats.Add(stat);
            }

        }
        catch (SQLDataReaderException ex)
        {
            logger.Log(ex);
            throw;
        }
    return stats;
    }
}

Example of Parameritized Query

public List<GlobalStat> GetStatsById(int id)
{
    var stats = new List<GlobalStat>();

    string sql = @"SELECT * from GlobalStats WHERE Id = @Id";
    using (SqlConnection conn = new SqlConnection(ConString))
    {
         conn.Open();
         using (SQLCommand command = new SqlCommand(sql, conn))
         {         
             command.Parameters.Add(new SqlParameter("Id", id));
             SqlDataReader reader = command.ExecuteReader();
             while (reader.Read())
             {
                  GlobalStat stat = new GlobalStat();
                  stat.Key = dr[0].ToString();
                  stat.Value = int.Parse(dr[1].ToString());

                  stats.Add(stat);
             }
         }
     }
     return stats;
}

Answer 3

Stored Procedures or LINQ to SQL are two popular database access paradigms within C#.

For more information regarding LINQ to SQL check out Using Linq to SQL on Scott Gu's Blog.