4

I want to refactor an existing class of almost 5000 lines but I'm having difficulty with the constructor. Right now it's something like the following(methods here are in reality 10-30 blocks of code )

public MyClass( MyObject o ) {
  if ( o.name.equalsIgnoreCase("a") ) {
    doSomething()
   } else  {
     doSomethingElse() 
   }
   commonCode()
   if (o.name.equalsIgnoreCase("a") ) {
     doSecondThing()
   } else {
     doOtherSecondThing() //almost identical to doSecondThing but with some extra steps that absolutely have to be done in this sequence
  }
 // more of the same

}

I considered using inheritance and breaking things up into functions that would be overridden where necessary but that feels messy to me. Is there a pattern that fits this use case? Incidentally any advice on refactoring legacy code would be more than welcome.

LiamRyan
  • 1,892
  • 4
  • 32
  • 61
  • Have a look at [How do I compare strings in Java?](http://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java) – Guy May 08 '16 at 09:32
  • `if ( o.name = "a" )` doesn't compile. – Boris the Spider May 08 '16 at 09:33
  • why do you feel that inheritence is messy? seems to me like inheritence + factory is a good "clean" OO solution – Sharon Ben Asher May 08 '16 at 09:36
  • This looks like a classic case of inheritance. Possibly an abstract factory pattern - move the `doSomething` to there. Without knowing more about the business logic; recommendations are difficult. – Boris the Spider May 08 '16 at 09:39
  • It seems messy to create an abstract parent with so much logic and then override some select methods in the children of that parent. They're too tightly coupled for my liking with this solution. I was hoping for some kind of established builder or component pattern – LiamRyan May 08 '16 at 09:40
  • I think the Go approach fits: composition over inheritance. You could have a look at the Strategy or the Delegaten pattern ( depending on the Rest oft the code). You shouldn't be afraid of having some of the common code parts duplicated, if everything else is so different – J. Dow May 08 '16 at 09:55

2 Answers2

2

You are exactly right. Refactoring like you described is called Replace Conditional with Polymorphism. Also you can look through on Chain-of-responsibility, Command or Strategy design patterns.

Alex
  • 139
  • 2
  • 8
2

If every object follows the following pattern:

if(conditionA)
    DoA();
else
    DoElse();
Common();
if(conditionA2)
    DoA2();
else if(conditionB2)
    DoB2();
else
    DoElse2();
Common2();

I'd advice you to have a common class that gathers handlers with conditions. This is roughly what I mean (Pseudo-code not java):

public interface IConditionalHandler 
{
    bool Condition();
    void Action();
}
public class ActionHandler
{
    private List<IConditionalHandler> m_FirstHandlers;
    private List<IConditionalHandler> m_SecondHandlers; //Or possibly use a list of lists
    public ActionHandler()
    {
        m_FirstHandlers = new ArrayList<>();
        m_FirstHandlers.add(new HandlerA1());
        m_FirstHandlers.add(new HandlerB1());
        m_SecondHandlers = new ArrayList<>();
        m_SecondHandlers.add(new HandlerA1());
        m_SecondHandlers.add(new HandlerB1());
    }
    void DoStuff()
    {
        for(IConditionHandler handler : m_FirstHandlers)
        {
             if(handler.Condition())
             {
                 handler.Action();
                 break;
             }
        }
        CommonA();
        for(IConditionHandler handler : m_SecondHandlers)
        {
             if(handler.Condition())
             {
                 handler.Action();
                 break;
             }
        }
    }
}

If you have lots of segment, a list of lists can include your common code as an exit-handler and contain all of the logic. You delegate the logic out to implementing classes, and shorten the actual code in your class. However, as far as efficiency goes you are going to kill both the instruction and data cache. If this isn't what you're looking for, then more than likely this is: Chain-of-Responsibility Pattern - Wikipedia

Joachim Olsson
  • 316
  • 1
  • 8
  • Thank you for the code sample but I was looking for pattern suggestions of which Alex provided more. I would accept both answers if possible, I've upvoted yours – LiamRyan May 08 '16 at 10:04