Implementation of the Strategy Pattern using an interface, abstract class and delegate

Question

The following class was designed to help create a more detailed error message than what's provided by the repository when a user tries to insert text into a column that is > the column max length. The users would like to be able to track down exactly which column(s) were the issue. Therefore, I created the following, and I would like to know if this is a good approach or not.

This class defines the parameters needed and is the context object for each respective strategy class.

public class QueryBuilder
{
public StringBuilder ColumnNames{get;private set;}
    public StringBuilder ColumnValues{get;private set;}
    public string TableName{get;private set;}
    public Database.FactoryType FactoryType {get;private set;}

    private static Dictionary<Database.FactoryType, IQueryBuilderStrategy> queryBuilderStrategies;
    public QueryBuilder(StringBuilder columnNames,StringBuilder columnValues,string tableName, Database.FactoryType factoryType)
    {
            TableName = tableName;
            ColumnNames = columnNames;
            ColumnValues = columnValues;
            FactoryType = factoryType;

            LoadStrategies();
    }
    private void LoadStrategies()
    {
        queryBuilderStrategies = new Dictionary<Database.FactoryType,IQueryBuilderStrategy>();
        queryBuilderStrategies.Add(Database.FactoryType.SqlClient, new SQLErrorQueryStrategy() );
        queryBuilderStrategies.Add(Database.FactoryType.OleDb, new AccessErrorQueryStrategy());
    }
    public string Create()
    {     
            return queryBuilderStrategies[FactoryType].CreateSQLQueryString(this);
    }

    public string Read(IDataReader reader)
    {
        return queryBuilderStrategies[FactoryType].ParseDataReader(this, reader);
    }
}

I created the following interface to decouple all the different strategies needed to communicate with multiple databases that we support.

public interface IQueryBuilderStrategy
{
    string CreateSQLQueryString(QueryBuilder queryBuilder);
    string ParseDataReader(QueryBuilder queryBuilder, IDataReader reader);
}

I then took it a step further and defined an abstract class because there was alot of duplication of code that only needed to change slightly

public abstract class ErrorQueryStrategyBase : IQueryBuilderStrategy
{
    protected Func<DataRow,bool> IsTextField;

    protected virtual string[] GetFieldNames(QueryBuilder builder)
    {
        return builder.ColumnNames.ToString().Replace("\"", string.Empty).Replace("'", string.Empty).Split(',');
    }
    protected virtual string[] GetFieldValues(QueryBuilder builder)
    {
        return builder.ColumnValues.ToString().Replace("'", string.Empty).Split(',');
    }
    protected virtual string GetErrorMessage(string fieldName, int currentFieldLength, string dbColumnName, int columnMaxSize)
    {
        string indexErrorMessage = String.Format("Field {0} length is {1}. However Data storage max length for column {2} is {3}{4}");
        return string.Format(indexErrorMessage,
                        fieldName, currentFieldLength, dbColumnName, columnMaxSize, System.Environment.NewLine);
    }

    public virtual string CreateSQLQueryString(QueryBuilder queryBuilder)
    {
        return string.Format("SELECT {0} FROM {1}", queryBuilder.ColumnNames.ToString(), queryBuilder.TableName);     
    }                       
    public virtual string ParseDataReader(QueryBuilder builder, IDataReader reader)
    {
        string[] userProvidedValues = GetFieldValues(builder);
        string[] columnNamesSeparated = GetFieldNames(builder);            
        string errorMessage = string.Empty;
        DataTable dt = reader.GetSchemaTable();
        int index = 0;
        foreach (DataRow row in dt.Rows)
        {
            string columnName = row[0].ToString();
            int columnSize = (int)row[2];
            int len = userProvidedValues[index].Length;
            if (IsTextField(row) )
            {
                if (len > columnSize)
                {
                    string fieldName = columnNamesSeparated[index];
                    errorMessage += GetErrorMessage(fieldName, len, columnName, columnSize);
                }
            }
            index++;
        }
        return errorMessage;
    }
}
public class SQLErrorQueryStrategy : ErrorQueryStrategyBase
{
    private readonly int textFieldIndex;
    public SQLErrorQueryStrategy()
    {
        this.textFieldIndex = 12;
        this.IsTextField = delegate(DataRow row)
        {
            Type dataType = (Type)row[textFieldIndex];
            return dataType.Name == "String";
        };
    }            
}
public class AccessErrorQueryStrategy : ErrorQueryStrategyBase
{
    private readonly int textFieldIndex;
    public AccessErrorQueryStrategy()
    {
        this.textFieldIndex = 5;
        this.IsTextField = delegate(DataRow row)
        {
            Type dataType = (Type)row[textFieldIndex];
            return dataType.Name == "String";
        };
    }
}

I instantiate and call the class here:

QueryBuilder errorQuery = new QueryBuilder(fieldList,valueList,tableName,capDatabase.Type);    
    message += GetDetailedErrorMessage(capDatabase, errorQuery);

private string GetDetailedErrorMessage(IDatabase database, QueryBuilder queryBuilder)
{
    string message = string.Empty;
    try
    {
        using (DbCommand errorQueryCommand =
            database.CreateCommand(queryBuilder.Create(), CommandType.Text))
        {
            errorQueryCommand.Connection = databaseConnection;          
            using (DbDataReader reader = errorQueryCommand.ExecuteReader(CommandBehavior.SchemaOnly))
            {
                message += queryBuilder.Read(reader);                             
            }
        }
    }
    catch (Exception ex)
    {
        message += ex.Message;
    }
    return message;
}

Using this approach, I was able to isolate the changes out. It also allows for future changes and additional db support.

Please provide constructive comments.

Answer 1

I think QueryBuilder can use constructor parameters, and that its auto-properties can be changed to be { get; private set; }. Otherwise the public setters seem to make an opportunity for some trouble. Actually I'd drop the auto-properties and initialize private readonly fields, exposed by get-only properties.

I don't get the static modifier for a dictionary that is recreated for every instance of the type. I would drop the static modifier altogether.

public Database.FactoryType Type {get;set;}

Might be a nitpick, but I find Type is a confusing name to use here, it clashes with System.Type. FactoryType would be much more appropriate.

Also there are inconsistencies in the way you're dealing with IDisposable in the try block of GetDetailedErrorMessage:

    DbCommand errorQueryCommand =
        database.CreateCommand(queryBuilder.Create(), CommandType.Text);            
    errorQueryCommand.Connection = databaseConnection;          
    using (DbDataReader reader = errorQueryCommand.ExecuteReader(CommandBehavior.SchemaOnly))
    {
        message += queryBuilder.Read(reader);                             
    }
    if (errorQueryCommand != null)
        errorQueryCommand.Dispose();

I don't see a reason not to do using (var errorQueryCommand = database.CreateCommand(...); also you're assigning the command's Connection to some databaseConnection which has to be a private field (that would be clearer if the name was _databaseConnection instead, but that could be only me).

One thing though, is that I don't think errorQueryCommand would ever be null where you're testing for it - if it were the case, you'd already be in the catch block over a NullReferenceException caused by accessing the setter of errorQueryCommand.Connection. But that point is moot if you wrap the DbCommand in a using block.

The rest looks ok, except if you're a fan of var, in which case reading your code... tickles. I personally find it very redundant to see string message = string.Empty; when it's obvious that message is a string - not because of its name, but because of the value it's being assigned to.

Answer 2

In addition to what @lol.upvote already said:

I don't really like the way you pass the column names and values around for several reasons:

1. You basically pass them as a string which means that whoever provides them very likely already has a list and needs to concatenate them together and then you have to split them again.

2. You rely on the fact that there are the same number of column names provided as there are column values. Your code will crash if that is not the case.

3. If a column name or value contains a , you are in trouble as well. Probably unlikely to be the case for the names but values might be more likely. From experience it's not a question of "if" but of "when" this is going to happen.

4. It violates the Single Responsibility Principle to a certain degree. This becomes apparent when you write unit tests because now you have to write unit tests to check that it can parse the names and value correctly - a functionality which is not really related to the actual purpose of the class.

So I'd suggest to change the QueryBuilder constructor like this:

public QueryBuilder(KeyValuePair<string, string> columnValues, string tableName, Database.FactoryType factoryType)
{
    ..

You'll have to adapt some code but you can also get rid of the parsing for the names and values.

Instead of a KeyValuePair you might want to consider a Tuple or your own ColumnValue class instead.