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.

• Wayne Conrad: Outstanding first post. Welcome!
• Jeff Vanzella: What's the reasoning behind using a delegate instead of a virtual method?
• monkeyjumps: Well the first pass was just that, two concrete classes implementing the interface IQueryBuilderStrategy then I realized they had a lot more in common then i originally planned. If I would have left them as virtual the concrete classes would have basically been a case of copy and paste. Therefore, I continued to re-factor.
• monkeyjumps: Please expand on why you think using a delegate may not be the best choice. I have been getting answers based on functional constructs but i was really in search of OOP critiques.

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.

• monkeyjumps: I updated my code to reflect the using statements you suggested. As far as the static dictionary goes i feel that is the pivotal point that makes this more DRY. My point being that as new provider support is requested then I only have to add to this dictionary here at a single point no more.
• Mathieu Guindon: But you're reassigning that dictionary everytime the constructor runs; whether it's static or not wouldn't make a difference then... unless you're reusing the same instance and calling these setters. I think it makes more sense for these values to be per-instance, so you can't accidentally miss a property and drag a value from another use. Don't give different meanings to the same variable; this applies to a string, just as it applies to a QueryBuilder IMHO. Feel free to upvote if any helpful - this isn't StackOverflow, we're a beta site trying to build a hi-rep user base and graduate :)
• monkeyjumps: I am sorry but i don't completely understand how I would be applying new meaning to QueryBuilder. Would you please expand? Can you provide an example how i can achieve my point without the dictionary?
• Mathieu Guindon: I didn't say drop the dictionary, I meant to drop the static keyword, it's not really buying you anything, but I see how it might be better off at the type level (vs. instance). By "new meaning" I mean, if you have an instance, and then you reuse that same instance later but for a different TableName and/or ColumnNames, or even a different Type - the public setters you have here allow for that instance to be misused and then your can change TableName and forget to update ColumnNames for example; with private setters and parameterized constructor you avoid that kind of trouble.
• monkeyjumps: I hear you. Good point.

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.

• monkeyjumps: Excellent point, definitely a sore thumb. In regards to your points {1,3} I guess I was trying to lie to myself by saying if i make the ColumnNames and ColumnValues methods virtual then i will be ok. However, that still doesn't help when the lists are not the same length as you pointed out as well.