CODEWITHSUNDEEP
c#design-patterns
romeozor
romeozor

Reputation: 941

Refactoring long if - else if - else with common objects

I'm looking for a way to refactor a log if/else if/else statement that also has a bit of nesting. The blocks also use quite a few common objects. My aim is to break the code apart to manageable units extracted to different classes and make it pluggable in case I need to cover a new condition.

Here's some dummy code to illustrate:

List<ValidationResult> validationResults = new ...;
Inspector inspector = commonInspector;
bool additionalOp = commonFlag;
HashSet<Thing> thingsToCheck = theThings;
foreach (var thing in thingsToCheck)
{
    if (IsCandy(thing) && thing.IsWrapped && thing.IsHard)
    {
        var inspected = inspector.Inspect(thing.Value);
        if (inspected != thing.Value)
        {
            validationResults.Add(new ...);
            if (additionalOp)
            {
                thing.Taste();
            }
        }
    }
    else if (IsChocolate(thing))
    {
        var sweet = (Sweet)thing;
        List<BadCalories> badCalories;
        while (var calorie in sweet.Calories)
        {
            if (calorie.IsGood)
                continue;
            badCalories.Add(calorie);
        }

        foreach (var badCal in badCalories)
        {
            var inspected = inspector.Inspect(badCal.Value);
            if (inspected != badCal.Value)
            {
                validationResults.Add(new ...);
                if (additionalOp)
                {
                    badCal.Taste();
                }
            }
        }
    }
    else
    {
        if(thing ...)
        else if (thing ...)
    }

I read a bunch of articles/SO posts of various patterns/practices that may apply, but the code's dependencies complicate it a bit for me to apply the concepts. It doesn't help that I've been looking at the code somewhat too closely for a while now so it's hard to break out from micro-managing to a birds eye view.

Upvotes: 7

Views: 1967

Answers (4)

TTat
TTat

Reputation: 1496

This is something the Strategy Pattern is for.

http://www.oodesign.com/strategy-pattern.html

You can do something like this:

List<ValidationResult> validationResults = new ...;
Inspector inspector = commonInspector;
bool additionalOp = commonFlag;
HashSet<IThing> thingsToCheck = theThings;
foreach (IThing thing in thingsToCheck)
{
    thing.Validate(inspector);
}

...

public interface IThing
{
    void Validate(Inspector inspector, ...);
}

public abstract CandyBase : IThing
{
    public abstract void Validate(Inspector inspector);
}

public HardWrappedCandy : CandyBase
{
    public override void Validate(Inspector inspector)
    {
         // HardWrappedCandy validation logic here
         var inspected = inspector.Inspect(this);
         ...
    }
}

public Chocolate : IThing
{
    public void Validate(Inspector inspector)
    {
        // Chocolate validation logic here...
    }
}

public SomethingElseInTheFuture: IThing
{
    public void Validate(Inspector inspector)
    {
        // SomethingElseInTheFuture validation logic here...
    }
}

Upvotes: 0

nullreff
nullreff

Reputation: 308

It looks like you're trying to pass common information from thing to inspector. In order to do this you need methods to:

  1. Retrieve objects to be inspected
  2. Inspect them

So lets define two interfaces:

One that can be used to inspect objects.

interface IInspectable
{
    string Value { get; }
    void Taste();
    // And any other methods that need to be called while inspecting
}

and another that can use to retrieve all objects from a thing that must be inspected.

interface IThing
{
    IEnumerable<IInspectable> GetStuffForInspection();
}

from there, we can implement our classes

class Candy : IThing, IInspectable
{
    public IEnumerable<IInspectable> GetStuffForInspection()
    {
        if (IsWrapped && IsHard)
            yield return this;
    }

    public String Value { ... }
    public void Taste() { ... }
}

class Chocolate : Sweet, IThing
{
    public IEnumerable<IInspectable> GetStuffForInspection()
    {
        Calories.Where(c => !c.IsGood);
    }
}

class Calorie : IInspectable
{
    public String Value { ... }
    public void Taste() { ... }
}

allowing you can remove all the if statements

foreach (var thing in thingsToCheck)
{
    foreach (var inspectable in thing.GetStuffForInspection())
    {
        var inspected = inspector.Inspect(inspectable.Value);
        if (inspected != inspectable.Value)
        {
            validationResults.Add(new ...);
            if (additionalOp)
            {
                inspectable.Taste();
            }
        }
    }
}

Upvotes: 0

Luc Morin
Luc Morin

Reputation: 5380

You should create an interface for all your "things" which declares the various checks that must be performed.

Then your "things" must implement said interface such that you can call the checks without worrying what the actual implementing type is.

//Interface
public interface ICheck
{
    bool Check1();
}

//Class
public Chocolate : ICheck
{
   public bool Check1()
   {
      //logic goes here
   }
}

Cheers

Upvotes: 0

Patrick Huber
Patrick Huber

Reputation: 756

You could break the large scope blocks into separate functions. 

if(IsHardWrappedCandy(thing))
  ValidateHardWrappedCandy(thing);
else if (IsChocolateCandy(thing))
  ValidateChocolateCandy(thing);

There is also interitance, where you would create candy classes and encapsulate behavior:

public abstract class CandyBase
{
    public abstract void Validate();
}
public class HardWrappedCandy : CandyBase
{
  public override void Validate()
  {
     // TODO: Validation logic
  }
}

Then your code would be:

foreach(var candy in candies)
   candy.Validate();

Of course you will need to standardize parameters and such, but you get the idea.

Read the book Clean Code, it has a lot of great ideas on how to refactor. http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

Upvotes: 7

Related Questions