Problem with removing "if" statement

Question

Hi,

I'm trying to find way how to improve this code. I would like to remove "if" statement from CreateAttributes method. The main idea of this method to add attribute to list if this attribute satisfies some conditions

  internal class FildMap
  {
    public string ExactTargetFild { get; set; }
    public string DbFild { get; set; }     
    public Type Type { get; set; }
  }

  internal static class FildMapProcessor
  {
    private static readonly List<FildMap> Map = new List<FildMap>();

    static FildMapProcessor()
    {
      if(Map.Count == 0)
      {
      Map.Add(new FildMap {ExactTargetFild = "Address 1", DbFild = "Address1", Type = typeof (string)});
      Map.Add(new FildMap { ExactTargetFild = "Date of birth", DbFild = "DateOfBirth", Type = typeof(DateTime) });
      Map.Add(new FildMap { ExactTargetFild = "Wine Beer", DbFild = "pref_WineBeerSpirits", Type = typeof(bool) });
      .........
      }
    }

    public static Attribute[] CreateAttributes(this DataRow row)
    {
      var attributes = new List<Attribute>();
      foreach (var item in Map)
      {
        if (item.Type == typeof(string))
        {
          var value = row.Get<string>(item.DbFild);
          if (value != null)
            attributes.Add(new Attribute{Name = item.ExactTargetFild, Value = value});
        }

        if (item.Type == typeof(DateTime))
        {
          var value = row.Get<DateTime>(item.DbFild);
          if (value != DateTime.MinValue)
            attributes.Add(new Attribute { Name = item.ExactTargetFild, Value = value.ToString("dd/MM/yyyy") });
        }

        if (item.Type == typeof(bool))
        {
          if (row.Contains(item.DbFild))
          {
            var value = row.Get<bool>(item.DbFild);
            attributes.Add(new Attribute { Name = item.ExactTargetFild, Value = value.ToString() });
          }
        }
      }

      return attributes.ToArray();
    }
  }

Thanks,

Answer 1

You can use polymorphism here

  internal abstract class FildMap
  {
    public string ExactTargetFild { get; set; }
    public string DbFild { get; set; }     
    public abstract List<Attributes> GetAttributes(DataRow row);
  }

  internal class StringFildMap : FildMap
  { 
    public override List<Attributes> GetAttributes(DataRow row)
    {
      //string specific stuff
    }
  }

create other classes for other types

public static Attribute[] CreateAttributes(this DataRow row)
{
  var attributes = new List<Attribute>();
  foreach (var item in Map)
  {
    attributes.AddRange(item.GetAttributes(row)); 
  }

  return attributes.ToArray();
}

Answer 2

the first quality improvement would be to use else if. It will mean the same thing for the example posted, but be a little clearer.

For this kind of thing I prefer using if

e.g.

if(){
} 
else if {
} 
else if {
} else {
}

The main alternative is switch (don't do the below because it'll fail):

        switch(typeof(PingItemRequestQrMsg))
        {
            case typeof(string):
                break;
        }

but this results in:

        Error   1   A switch expression or case label must be a bool, char, string, integral, enum, or corresponding nullable

Which means you need an extra level on indirection. Like you tag each class through an interface e.g. IMyTag which returns an enum of some kind, or you assign ids to each type via a helper function.

Seriously, if is normally not a bad choice for this kind of thing. Often it's smarter to endure using an if than to endure the lack of compile-time type checking that can result from a switch using a string approach. i.e. get one letter wrong and your switch statement doesn't appear to be quite so cool any more.

If there's something generic going on inside each conditional branch, there are other options.

Answer 3

You could build a map between types and integers. Then you could switch on the result of the lookup. On SO is another question with that approach posted.

A copied part of one of the answers:

Dictionary<Type, int> typeDict = new Dictionary<Type, int>
{
    {typeof(int),0},
    {typeof(string),1},
    {typeof(MyClass),2}
};

void Foo(object o)
{
    switch (typeDict[o.GetType()])
    {
        case 0:
            Print("I'm a number.");
            break;
        case 1:
            Print("I'm a text.");
            break;
        case 2:
            Print("I'm classy.");
            break;
        default:
            break;
    }
}

Credits for that code goes to bjax-bjax.

Maybe you could think of wrapping the lookup to a function to return a default value.

Answer 4

I think its better to refactor your code as follow:

internal class FildMap
{
    public string ExactTargetFild { get; set; }
    public string DbFild { get; set; }
    public Type Type { get; set; }
    public  object GetValue(object value)
    {
        switch(Type.Name)
        {
            case "System.String":
                // [Code]
                break;
            case "System.DateTime":
                // [Code]
                break;
            case "System.Boolean":
                // [Code]
                break;
        }

        return null;
    }


}

internal static class FildMapProcessor
{
    private static readonly List<FildMap> Map = new List<FildMap>();

    static FildMapProcessor()
    {
        if (Map.Count == 0)
        {
            Map.Add(new FildMap { ExactTargetFild = "Address 1", DbFild = "Address1", Type = typeof(string) });
            Map.Add(new FildMap { ExactTargetFild = "Date of birth", DbFild = "DateOfBirth", Type = typeof(DateTime) });
            Map.Add(new FildMap { ExactTargetFild = "Wine Beer", DbFild = "pref_WineBeerSpirits", Type = typeof(bool) });
        }
    }

    public static Attribute[] CreateAttributes(this DataRow row)
    {
        var attributes = new List<Attribute>();
        foreach (var item in Map)
        {
            foreach (var item in Map)
            {
                var value = item.GetValue(row[item.DbFild]);
                if(value != null)
                    attributes.Add(new Attribute { Name = item.ExactTargetFild, Value = value });
            }
        }           

        return attributes.ToArray();
    }
}

Answer 5

a case switch would be better:

switch (item.Type)
{
    case typeof(string):
        // code here
       break;
    case typeof(DateTime):
        // code here
       break;
    case typeof(bool):
        // code here
       break;
}