I posted a more detailed version of this question on code review exchange, check this link. I later got my implementation reviewed by one of my colleagues and he suggested a different approach to solve the problem. So this question is to help me find out, which of the two approaches should be picked and why?
If this is not suitable for StackOverflow and should still be in CodeReviewExchange, please let me know and I will move it there.
Also I want to say this in the beginning itself because I was criticized for this in my earlier post. The actual code is different than what I have provided here, its more complex so I had to simplify it to ask just what I want to figure out i.e. inheritance vs composition, which is better here?
Here is my set of classes:
public abstract class ConditionBuilder<TContext> : IConditionBuilder where TContext : FieldSearchContext
{
public virtual bool QuotedValues { get; set; } = true;
public abstract string OperatorSymbol { get; }
public string BuildCondition(SearchCondition searchCondition)
{
var conditionBuilder = new StringBuilder();
var context = searchCondition.GetContext<TContext>();
conditionBuilder.Append(context.FieldId);
conditionBuilder.Append(OperatorSymbol);
conditionBuilder.Append(GetValue(context));
return conditionBuilder.ToString();
}
public abstract bool CanHandle(FilterAction filterAction);
public abstract object GetValue(TContext context);
}
public class TextLikeConditionBuilder : ConditionBuilder<TextContext>
{
public override string OperatorSymbol => " LIKE ";
public override bool CanHandle(FilterAction action) => action == FilterAction.TextLike;
public override object GetValue(TextContext context)
{
if (context.Text == null)
{
return null;
}
return string.Concat("%", context.Text, "%");
}
}
public class TextEqualsConditionBuilder : ConditionBuilder<TextContext>
{
public override string OperatorSymbol => " = ";
public override bool CanHandle(FilterAction action) => action == FilterAction.TextEqual;
public override object GetValue(TextContext context)
{
if (context.Text == null)
{
return null;
}
return "'" + context.Text + "'";
}
}
In their default implementation the classes build a WHERE clause for SQL. These classes are consumed by other developers and can be overridden to build WHERE clause that is not necessarily meant for Database for example one can override protected virtual StringBuilder GetCondition(SearchFilterCondition filterCondition, TContext context) to build a QUERY for Elasticsearch also.
Coming back to the topic, in one particular instance where these classes are used with their default behavior, I wanted to delimit the FieldId so as to ensure that it works when FieldId contains spaces and special characters. This is how I implemented the solution:
public interface IDelimitedIdentifier
{
string Delimit(string input);
}
internal class SqlServerDelimitedIdentifier : IDelimitedIdentifier
{
public string Delimit(string input)
{
return "[" + input.Replace("]", "]]") + "]";
}
}
internal class OracleDelimitedIdentifier : IDelimitedIdentifier
{
public string Delimit(string input)
{
return "\"" + input + "\"";
}
}
public abstract class ConditionBuilder<TContext> : IConditionBuilder where TContext : FieldSearchContext
{
public virtual bool QuotedValues { get; set; } = true;
public abstract string OperatorSymbol { get; }
public string BuildCondition(SearchCondition searchCondition)
{
var conditionBuilder = new StringBuilder();
var context = searchCondition.GetContext<TContext>();
conditionBuilder.Append(SanitizeFieldId(context.FieldId));
conditionBuilder.Append(OperatorSymbol);
conditionBuilder.Append(GetValue(context));
return conditionBuilder.ToString();
}
public abstract bool CanHandle(FilterAction filterAction);
public abstract object GetValue(TContext context);
protected virtual string SanitizeFieldId(string fieldId)
{
return _delimitedIdentifier.Delimit(fieldId);
}
}
public class SanitizedFieldConditionBuilder<TContext> : ConditionBuilder<TContext> where TContext : FieldSearchContextBase
{
private readonly ConditionBuilder<TContext> _baseConditionBuilder;
private readonly IDelimitedIdentifier _delimitedIdentifier;
public SanitizedFieldConditionBuilder(ConditionBuilder<TContext> baseConditionBuilder, IDelimitedIdentifier delimitedIdentifier)
{
QuotedValues = false;
_baseConditionBuilder = baseConditionBuilder;
_delimitedIdentifier = delimitedIdentifier;
}
public override string OperatorSymbol => _baseConditionBuilder.OperatorSymbol;
public override bool CanHandle(SearchFilterAction action) => _baseConditionBuilder.CanHandle(action);
public override object GetValue(TContext context) => _baseConditionBuilder.GetValue(context);
protected override string SanitizeFieldId(string fieldId)
{
return _delimitedIdentifier.Delimit(fieldId);
}
}
public static class ConditionBuilderExtensions
{
public static SanitizedFieldConditionBuiler<TContext> SanitizeField<TContext>(this ConditionBuilder<TContext> source, IColumnSanitizer columnSanitizer) where TContext : FieldSearchContext
{
return new SanitizedFieldConditionBuiler<TContext>(source, columnSanitizer);
}
}
The classes can be instantiated as shown below:
class Program
{
static void Main(string[] args)
{
var conditionBuilders = new List<IConditionBuilder>()
{
new TextEqualsConditionBuilder().SanitizeField(new SqlServerDelimitedIdentifier()),
new TextLikeConditionBuilder().SanitizeField(new SqlServerDelimitedIdentifier())
};
}
}
My colleague suggested to use composition instead of extension. Here is how the classes would look like as per his recommendation.
public abstract class ConditionBuilder<TContext> : IConditionBuilder where TContext : FieldSearchContext
{
private readonly IDelimitedIdentifier DelimitedIdentifier;
public virtual bool QuotedValues { get; set; } = true;
public abstract string OperatorSymbol { get; }
protected ConditionBuilder(IDelimitedIdentifier delimitedIdentifier)
{
DelimitedIdentifier = delimitedIdentifier;
}
public string BuildCondition(SearchCondition searchCondition)
{
var conditionBuilder = new StringBuilder();
var context = searchCondition.GetContext<TContext>();
conditionBuilder.Append(DelimitedIdentifier.Delimit(context.FieldId));
conditionBuilder.Append(OperatorSymbol);
conditionBuilder.Append(GetValue(context));
return conditionBuilder.ToString();
}
public abstract bool CanHandle(FilterAction filterAction);
public abstract object GetValue(TContext context);
}
public class TextLikeConditionBuilder : ConditionBuilder<TextContext>
{
public TextLikeConditionBuilder(IDelimitedIdentifier delimitedIdentifier) : base(delimitedIdentifier)
{
}
public override string OperatorSymbol => " LIKE ";
public override bool CanHandle(FilterAction action) => action == FilterAction.TextLike;
public override object GetValue(TextContext context)
{
if (context.Text == null)
{
return null;
}
return string.Concat("%", context.Text, "%");
}
}
public class TextEqualsConditionBuilder : ConditionBuilder<TextContext>
{
public TextEqualsConditionBuilder(IDelimitedIdentifier delimitedIdentifier) : base(delimitedIdentifier)
{
}
public override string OperatorSymbol => " = ";
public override bool CanHandle(FilterAction action) => action == FilterAction.TextEqual;
public override object GetValue(TextContext context)
{
if (context.Text == null)
{
return null;
}
return "'" + context.Text + "'";
}
}
Here are my arguments for why it shouldn't be done as per what my colleague recommends doing:
IDelimitedIdentifier is very specific to database, why should it be moved inside the base class when the functionality can be supported using extension.
- Extension reduces the number of changes otherwise adding that constructor would mean changing all derived classes and let me tell you that there are 15 odd deriving classes
- Isn't adding a constructor against Open-Closed principle?
