Refactor to a better code

On today’s show: how to massively improve a piece of code.

Old method

Take a glimpse on this given function (you don't have to read all 😄):

public  string BuildFilterQuery(string Xmlquery)
        {
            var query = string.Empty;
            var doc = new XmlDocument();
            var values = string.Empty;
            doc.LoadXml(Xmlquery);
            
                try
                {
                    string fieldID;
                    string conditionOperator;
                    string conditionValues = string.Empty;
                    XmlNodeList filters = doc.SelectNodes("//Root//Filter");
                    for (int i = 0; (i
                                     <= (filters.Count - 1)); i++)
                    {
                        XmlNodeList root = filters[i].SelectNodes(".//FilterItem");
                        query = (query + Convert.ToString("("));
                        foreach (XmlNode item in root)
                        {
                            fieldID = item.SelectSingleNode("FieldID").InnerText;
                            conditionOperator = item.SelectSingleNode("Operator").InnerText.ToLower();
       conditionValue = item.SelectSingleNode("Operator").InnerText;
                            
                                switch (conditionOperator)
                                {
                                    case "contains":
                                        query = string.Format("{0} [{1}] {2} \'%{3}%\' AND", query, DataManipulate.ConvertFromSafe(fieldID), dOperators[conditionOperator], conditionValue);
                                        break;
                                    case "after":
                                        query = string.Format("{0} Convert([{1}], \'System.DateTime\') {2} \"#{3}#\" AND", query, DataManipulate.ConvertFromSafe(fieldID), dOperators[conditionOperator], conditionValue);
                                        break;
                                    case "greater than":
                                        query = string.Format("{0} Convert([{1}], \'System.Int64\') {2} {3} AND", query, DataManipulate.ConvertFromSafe(fieldID), dOperators[conditionOperator], conditionValue);
                                        break;
                                    default:
                                        query = string.Format("{0} [{1}] {2} \'{3}\' AND", query, DataManipulate.ConvertFromSafe(fieldID), dOperators[conditionOperator], conditionValue);
                                        break;
                                }                            

                        }
      
                        if (query.EndsWith(" AND"))
                        {
                           query = (query.Substring(0, query.LastIndexOf(" AND")) + ")");
                        } 

                        query = (query + " OR ");
                    }
                }
                catch (Exception ex)
                {
                    throw ex;
                }            

            return query;
        }

Basically - this function iterates an xml which contains query details and returns one single query string.

What’s wrong with that function?

      1. Code handles xml object instead desterilize to a convenient and more readable object.

      2. It’s long and not fun to read by other developers.

      3. Code contains many different logics (responsibilities) which can probably be reused across the

      application. In addition, adding too much logic makes it less suitable for unit-testing.

To test this method – one should create many different xml strings and execute it.

But what if there is an issue with iterating the xml? All tests would fail – and we wouldn’t know immediately why. Much logic causes failure reason range to be wide.

      4. Code is hard for extension. Adding another “case” to “switch” means modifying a working method.

      5. Because method is long – it’s hard to maintain by a development team. Chances of code conflicts are bigger.

      6. On a performance matter - string type is immutable and query gets changed again and again (better use a StringBuilder object).

     Code surgery!

     
Deserialize to custom C# object

       1^st paragraph is the easiest step. A proper C# class generated from xml structure. There are various   ways of doing that. Personally, I like to use an online tool like http://xmltocsharp.azurewebsites.net/ .

       Here is part of the new class:

       [XmlRoot(ElementName = "Filter")]
    public class Filter
    {
        [XmlElement(ElementName = "FilterName")]
        public string FilterName { get; set; }
        [XmlElement(ElementName = "FilterItem")]
        public List<FilterItem> FilterItem { get; set; }
    }

      

     Method breakdown

     Switch-case breakdown

     Easy to identify that there is kind of a switch-case on operator. The purpose of each 

     if-statement is to create a query string in a certain format.

     IQueryStringType consists one method which gets relevant data and returns a query string.

   

public interface IQueryStringType
{
  string ConcatQueryString(QueryStringTypeModel queryStringTypeModel);
}

QueryStringTypeModel consists relevant information to create a query string.

public class QueryStringTypeModel
{
  public string FilterName { get; set; }
  public string Operator { get; set; }
  public List<string> ValueList { get; set; }
  public Dictionary<string, string> TranslateOperators { get; set; }
  public QueryStringTypeSettingsModel QueryStringTypeSettingsModel { get; set; }
}

Here is an example of a query string type implementation:

public class ContainsQueryStringType : IQueryStringType
{   
  public string ConcatQueryString(QueryStringTypeModel queryStringTypeModel)
  {
    var convertedFromSafe = $"[{DataManipulate.ConvertFromSafe(queryStringTypeModel.FilterName)}]";
    var concatOperator = $"{queryStringTypeModel.TranslateOperators[queryStringTypeModel.Operator]}";
    var value = $"'%{queryStringTypeModel.ValueList[0]}%'";
    var response = $"{convertedFromSafe} {concatOperator} {value}";
    return response;
  }        
}

QueryStringTypeModel object is generated by QueryStringTypeModelGenerator.

This object also contains QueryStringTypeSettingsModel property which holds the required QueryStringType for current calculation.

QueryStringTypeSettingsModelRepository class holds all types of QueryString classes. GetModel(string @operator) method returns the appropriate instance of QueryStringTypeSettingsModel according to a given operator.

Shortend version of this repository class (only with "Contains"):

public class QueryStringTypeSettingsModelRepository
{
  private readonly Dictionary<string, QueryStringTypeSettingsModel> _queryStringTypeSettingsModelDic = new Dictionary<string, QueryStringTypeSettingsModel>();

  public QueryStringTypeSettingsModelRepository()
  {
    var containsSettingsModel = new QueryStringTypeSettingsModel()
    {
       QueryStringType = new ContainsQueryStringType(),
    };
    

    this._queryStringTypeSettingsModelDic.Add("Contains", containsSettingsModel); 
    this._queryStringTypeSettingsModelDic.Add("Not contains", containsSettingsModel);
  }

  public QueryStringTypeSettingsModel GetModel(string @operator)
  {
     if (!this._queryStringTypeSettingsModelDic.ContainsKey(@operator))
       return null;

     var queryStringTypeSettingsModel = this._queryStringTypeSettingsModelDic[@operator];
     return queryStringTypeSettingsModel;
  }
}

    

      If-statements gets activated only when condition is fulfilled

QueryStringCalculator checks for these conditions and activates ConcatQueryString of a the requested IQueryStringType sent.

public class QueryStringCalculator : IQueryStringCalculator
{
   public string GetQueryString(QueryStringTypeModel queryStringTypeModel)
   {
     var response = string.Empty;
     if (queryStringTypeModel.QueryStringTypeSettingsModel == null 
         || string.IsNullOrEmpty(queryStringTypeModel.FilterName)
         || string.IsNullOrEmpty(queryStringTypeModel.Operator))
                return response;

     var queryStringType = queryStringTypeModel.QueryStringTypeSettingsModel.QueryStringType;
     response = queryStringType.ConcatQueryString(queryStringTypeModel);
     return response;
   }
}

      Merge FilterItems into a single query string (with “AND” between each couple)

Each of FilterItems is a filter statement and all of them should be united to one single query string with an “AND” operator between them. This logic is encapsulated into a new class.

public class FilterItemToQueryStringConvertor : IFilterItemToQueryStringConvertor
{
  private readonly IQueryStringCalculator _queryStringCalculator;
  private readonly IQueryStringTypeModelGenerator _queryStringTypeModelGenerator;

  public FilterItemToQueryStringConvertor(IQueryStringCalculator queryStringCalculator,
            IQueryStringTypeModelGenerator queryStringTypeModelGenerator)
  {
    _queryStringCalculator = queryStringCalculator;
    _queryStringTypeModelGenerator = queryStringTypeModelGenerator;
  }

  public string GetQueryString(List<FilterItem> filterItems, 
            Dictionary<string,string> dOperators)
  {
    var response = string.Empty;
    if (filterItems.Count == 0)
      return response;

    var queryStringCollection = new List<string>();

     foreach (var filterItem in filterItems)
     {
       var queryStringTypeGeneratorModel = new QueryStringTypeGeneratorModel
       {
          FilterItem = filterItem,
          TranslateOperators = dOperators
       };

       var queryStringTypeModel = _queryStringTypeModelGenerator.GetQueryStringTypeModel(queryStringTypeGeneratorModel);
       var queryString = _queryStringCalculator.GetQueryString(queryStringTypeModel);
       queryStringCollection.Add(queryString);
     }

    response = queryStringCollection.Aggregate((current, next) => current + " AND " + next);
    return response;
   }
}

Merge each all query strings into one (with “OR” between each couple)

All query strings that were generated using FilterItemToQueryStringConvertor should be united into a single query string.

public class FilterCollectionToQueryStringConvertor : IFilterCollectionToQueryStringConvertor
{
        private readonly IFilterItemToQueryStringConvertor _filterItemToQueryStringConvertor;

        public FilterCollectionToQueryStringConvertor(IFilterItemToQueryStringConvertor filterItemToQueryStringConvertor)
        {
            this._filterItemToQueryStringConvertor = filterItemToQueryStringConvertor;
        }

        public string GetQueryString(List<Filter> filters, 
            Dictionary<string, string> dOperators)
        {
            var response = string.Empty;
            if (filters.Count == 0)
                return response;
            
            var queryStringCollection = new List<string>();
            foreach (var filter in filters)
            {
                var queryString = _filterItemToQueryStringConvertor.GetQueryString(filter.FilterItem, dOperators);
                queryStringCollection.Add(queryString);
            }

            response = queryStringCollection.Aggregate((current, next) => current + " OR " + next);
            return response;
        }
}

    Summary + Tips

   
If you wish to enchance your coding level:

      

      a. Before writing any code - think about your code infrastracture.
Even during\after code writing - try to figure out if it's possible to encapsulate it and make it more generic.

      

      b. Read about SOLID principles:
https://www.codeproject.com/Articles/703634/SOLID-architecture-principles-using-simple-Csharp .
https://www.c-sharpcorner.com/UploadFile/damubetha/solid-principles-in-C-Sharp/

      

      

      c. Write your code that it would be able to use dependency injection (For instance: don't use concrete types\model parameters in constructors, create an interface for each class..).
Dependency injection is a way to create dependencies outside of the class using it. It allows developers to create decoupled & testable applications.

      SimpleInjector is a great DI library backed up with very informative documentation even about developing concepts:

      http://simpleinjector.readthedocs.io/en/latest/quickstart.html .