How would you refactor this conditional to use polymorphism?

Question

I just finished watching the Google clean code video on YouTube (see link, first article) about removing if statements from your code and using polymorphism instead.

After watching the video I had a look at some code that I was writing before watching the video and noticed some places where I could use this method, mainly places where the same kind of logic was implemented many times. So a example:

I have some code like this.

public int Number
{
    get
    {
        string returnValue;
        if (this.internalTableNumber == null)
             returnValue = this.RunTableInfoCommand(internalTableName,
                                                    TableInfoEnum.TAB_INFO_NUM);
        else
             returnValue = this.RunTableInfoCommand(internalTableNumber.Value,
                                                    TableInfoEnum.TAB_INFO_NUM);
        return Convert.ToInt32(returnValue);
    }
}

What RunTableInfoCommand does isn't really important,but the main thing is that I have many properties with exactly the same if statments the only thing that changes is the TableInfoEnum.

I was wondering if someone could help me refactor this so that it still does the same thing but without any if statements?

Answer 1

I would maybe consider passing off the fetching of the return value to another class that could be injected at runtime.

public class Thing
{
  public IValueFetcher ValueFetcher { get; set; }

  public int Number 
  { 
    get 
    {
      return this.ValueFetcher.GetValue<int>(/* parameters to identify the value to fetch */);
    }
  }
}

That would take care of a lot of the repetitive code and reduce your dependence on the source of the value to an interface.

I think at some point you're likely to have an if statement as you still need to decide which version of RunTableInfoCommand you want to call.

Answer 2

You will actually be implementing something like the Strategy pattern. Start by defining a super class lets call it AbstractTableInfoCommand. This class may be abstract but must specify a method called runTableInfoCommand().

You can then define several sub classes that each implement runTableInfoCommand() method. Your class, the one with the Number property, will then have a new property of type AbstractTableInfoCommand (lets call it tableInfoCommand) which will be instantiated to one of the concrete sub classes of AbstractTableInfoCommand.

The code will then be:

public int Number
    {
        get
        {

            return this.tableInfoCommand.runTableInfoCommand();
        }
    }

So you can create a NullTableInfoCommand and SomeOtherTableInfoCommand etc. The advantage is that if you have some new condition for returning a tableinfocommand then you add a new class rather than edit this code.

Having said that, not every situation is necessarily right for this pattern. So it makes more extendable code but if you are in a situation that does not require that extendability it mioght be overkill.

Answer 3

I assume internTableName and InternalTableNumber are some sort of value for the same thing. Why not wrap it up inside a class and pass a instance of that class to this.RunTableInfoCommand like so:

public int Number
        {
            get
            {
                string returnValue;
                internalTableClass myinstance(parameters);
                return Convert.ToInt32(this.RunTableInfoCommand(myinstance, TableInfoEnum.TAB_INFO_NUM));
            }
        }

if you still want to use polymorphism then, you can do so in that class by overloading for example a giveInternalTableIdentifier that returns the number or the name

the code here could then look like this:

public int Number
        {
            get
            {
                string returnValue;
                internalTableClass myinstance(parameters);
                return Convert.ToInt32(this.RunTableInfoCommand(myinstance.giveInternalTableIdentifier, TableInfoEnum.TAB_INFO_NUM));
            }
        }

and the code for the internalTableClass will be trivial (using: internalAbstractTableClass, and two classes that inherit from it, one giving the name, and the other the number)

Answer 4

EDIT 2 How I'd really solve the problem.

I'd make InternalTableNumber a property that is lazy loaded. If it's not available then I would look it up via the InternalTableName. Then I would always just use the InternalTableNumber property for my methods.

 private int? internalTableNumber;
 private int InternalTableNumber
 {
     get
     {
         if (!internalTableNumber.HasValue)
         {
             internalTableNumber = GetValueFromTableName( internalTableName );
         }
         return internalTableNumber;
     }
     set
     {
         internalTableNumber = value;
     }
 }

 public int Number
 {
     get
     {
        string value = this.RunTableInfoCommand(InternalTableNumber,
                                                TableInfoEnum.TAB_INFO_NUM);
        return Convert.ToInt32( value );
     }
 }

EDIT Using polymorphism...

Let's assume that your current class is named Foo, then I would refactor it into two classes, FooWithName and FooWithNumber. FooWithName would be the class you would use when you have the table name and FooWithNumber would be the class to use when you have the table number. I'd then write each class with a Number method -- actually, I'll write an interface IFoo as well that each implements so that they can be used interchangably.

public interface IFoo
{
     int Number { get; }|

}

public class FooWithName : IFoo
{
     private string tableName;
     public FooWithName( string name )
     {
         this.tableName = name;
     }

     public int Number
     {
        get { return this.RunTableInfoCommand(this.tableName,
                                       TableInfoEnum.TAB_INFO_NUM);
     }

     ... rest of class, including RunTableInfoCommand(string,int);
}

public class FooWithNumber : IFoo
{
     private int tableNumber;
     public FooWithNumber( int number )
     {
         this.tableNumber = number;
     }

     public int Number
     {
        get { return this.RunTableInfoCommand(this.tableNumber,
                                       TableInfoEnum.TAB_INFO_NUM);
     }

     ... rest of class, including RunTableInfoCommand(int,int);
}

The you would use it as so:

IFoo foo;

if (tableNumber.HasValue)
{
    foo = new FooWithNumber( tableNumber.Value );
}
else
{
    foo = new FooWithName( tableName );
}

int number = foo.Number;

Obviously, unless you have a lot of the if-then-else constructs in your existing class, this solution doesn't actually improve it much. This solution creates IFoo using polymorphism and then just uses the interface methods without caring about the implementation. This could easily be extended to inherit a common implementation of RunTableCommand( int ) in an abstract class that inherits IFoo and is the base class for FooWithNum and FooWithName.

Answer 5

I’d refactor it thusly:

table = this.internalTableNumber == null ? internalTableName : internalTableNumber.Value;
return Convert.ToInt32(this.RunTableInfoCommand(table, TableInfoEnum.TAB_INFO_NUM));

Love the ternary operator.

Answer 6

Just a cautionary note here after seeing some of these (technically correct) reponses, just getting rid of an If statement should not be your sole aim, the aim should be to make your code extensible, maintainable and simple, if that means getting rid of an if statement, great, but it shouldn't be an aim in an of itself.

In the code sample you have given, and without knowing more about your app, and assuming you are not going to extend much past testing for a null value, I think an If (or perhaps even a ternary) is the more maintainable solution to be perfectly frank.

Answer 7

I feel your code is perfectly fine. Readable. Simple. (And I hope it works). If you have this code block repeating n times, you need to remove duplication by applying Extract method.

The refactoring you indicate is meant to replace recurring switch cases.. not simple if statements like in your example. Replace Conditional With Polymorphism. Remember the 'simplest thing that works'.. which means the minimal number of classes and methods required to get the job done.

Answer 8

Going for a refactoring that involves polymorphism might be overkill in this case (depending on what other advantages you might get out of the polymorphism). In this case, adding a simple overload that encapsulates the logic of which RunTableInfoCommand() to call might be in order.

Since RunTableInfoCommand(), internalTableNumber, and internalTableName all seem to be members of the same same class, a nice, easy refactoring might be to add an overload of RunTableInfoCommand() that simply takes a TableInfoEnum value and does the logic of figuring out which other RunTableInfoCommand() overload needs to be called:

private string RunTableInfoCommand( TableInfoEnum infoEnum)
{
    if (this.internalTableNumber == null) {
        return this.RunTableInfoCommand( internalTableName, infoEnum);
    }

    return this.RunTableInfoCommand( internalTableNumber.Value, infoEnum);
}

Then the numerous call sites that have the same if decision logic can be collapsed to:

returnValue = this.RunTableInfoCommand( TableInfoEnum.TAB_INFO_NUM);    
                                        // or whatever enum is appropriate

Answer 9

how would i refactor these particular if statements to use polymorphism?

Well...I wouldn't. You don't need polymorphism to eliminate the if statements.

Note that the if statements as not 'bad' per se, the if statements are caused by the representation choice where

(internalTableNumber == null) ==> 
    internalTableName else internalTableNumber.Value

This association implies a missing class, i.e. it would make more sense to have an InternalTable class that owned internalTableName and internalTablenumber, since these two values are strongly associated by the null-check rule.

• This new class could provide a TableNumber property that performed the internalTableNumber == null check
• and the RunTableInfoCommand could take an InternalTable instance as a parameter (but it doesn't have to, see next item).
• But even better, the new class should have a facade to the RunTableInfoCommand method (which is presumably static) that performed the integer conversion.

Assuming the InternalTable instance is named iTable, the refactored code of concern would then look like:

public int Number
{
    get
    {
        return iTable.RunTableInfoCommand(TableInfoEnum.TAB_INFO_NUM);
    }
}

This would encapsulate the null-check in the InternalTable class. Modifying the original RunTableInfoCommand signature is optional.

Note that this does not "replace the if statements with polymorphism", but it does eliminate the if statements from the consuming class via encapsulation.