Refactoring: Two big chunks within a function

Question

I am designing an API which mostly involves refactoring the original code.

So Right now I have a method which has two big chunks which are seperated by an If-else condition, which in my opinion is not exactly the best idea.

The code looks like

do_something():
     if (isTrue):
         #Large block of statements.
     else:
         #another large block of statements.

The reason I have them both under a single function is because both the chunks do the same thing but with some slight variations, which introduces the ugly if-else block.

I wanted to know what would be the best idea to refactor this code in a better way, if it is possible to do that using OOP, that would be even better.

I am not going with the obvious defining two methods way because at the moment both the chunks are doing the same thing.

Answer 1

If you don't want to start with the obvious thing, you can also start with identifying small, common portions in block A and B and refactor each portion to a separate method which is then called from A as well as from B. Ideally, this will lead you to new blocks A and B only containing non-common functionality.

But then moving those functionality to separate methods will most probably still be a good idea, and the final result will be the same as if you did it the other way round.

To my experience, extracting small, common functions from A and B first has the advantage that you can deal with small methods which are quite often easier to understand, so the chance of introducing errors is smaller. Doing this step-by-step reduces also the size of the blocks A and B, and increases their readability, so afterwards the refactoring of A and B to methods gets easier.

Answer 2

The obvious way is a good way to start. Once you have methods a and b defined, you can extract their commonality into c - at which point maybe you can re-inline a and b into the original method.

Answer 3

You could model your "if" condition as two different classes. I'm assuming there are 3 differences between your "large blocks of statements." You might call one OneWay and the other AnotherWay.

Class OneWay(ParentWay):
    myWay(a, b, c):
        #Large block of statements.

Class AnotherWay(ParentWay):
    myWay(a, b, c):
         #another large block of statements.

So here's how you'd use these two classes

do_something(someWay):
    someWay.myWay(a, b, c)

Now it will do the right thing based on what type of class it's passed. This kind of turns things inside out.

I'm not saying one way is better than the other, but it depends on the situation. Certainly if your entire method is one big if/else statement and the if-block and else-blocks are nearly identical, then some kind of refactoring is probably called for.

A simpler and possibly better solution is this:

do_true_thing():
     #Large block of statements.

do_false_thing():
     #another large block of statements.

So instead of passing a true/false, the client calls an appropriate method. You would use better names. In general, I don't like to have booleans in method signatures, unless it's really obvious what they mean. When you get a method like do_stuff(isTrue, isReallyTrue) I have a hard time remembering what the parameters mean.

Using either of these techniques, you may still want to break the large blocks up into different procedures the way @CarlManaster suggested.

Pardon my pseudocode, I really don't know Python, but the concepts should still apply.

Answer 4

First things first, are there automated tests around this code already? Assuming not, is there a way you can write an automated test around this code to make sure you don't break it when you refactor it? You'll probably save time in the long run if you do this first.

After you've got that, I'd do what @CarlManaster. Extract them into two methods and piece by piece extract the parts they've got in common into a single method that they both share. It may help if you've got a way to compare the two methods with a diff tool. I use intellij-idea and it makes this pretty easy to do.

Answer 5

you could move the 2 blocks into seperate functions then use a simple dispatcher:

def do_something():
    function_1() if isTrue else function_2()

1. This will prevent haivng 2 "ugly" blocks of code
2. It is it a bad idea to treat functions as "bags of code" - if the code does differentoncerns then seperate them.
3. It will make the code more readable and pythonic - future you and maintainers will thanks you

Answer 6

I've seen code like this in the past, with "the large block of statements" being many hundreds of lines of code.

For starters, any "large block of statements" is, in itself, a code smell. As some of the other posters have mentioned, you need to break this down into smaller chunks of code.

As for handling the differences, there's no shame in using multiple if-else blocks on the same condition. Writing something like:

do_something():
    if(condition):
        do_stuff()
    else:
        do_other():
    # shared code
    if(condition):
        make_thing()
    else:
        make_other()
    # shared code
    if(condition):
        return this_stuff()
    else:
        return that_stuff()

If you're lucky, the differences really just come down to a few constants and you can clean up this mess by writing

do_something():
    if(condition):
        a = 10
        b = -1
        c = 1.5
    else:
        a = 100
        b = +1
        c = 1.9
    # shared code

Answer 7

You're saying the two chunks "do the same thing but with some slight variations". Are the variations small enough that you could replace each of the chunks with the same function, controlled by slightly different arguments? Don't do this if it would create even uglier code with lots of if/then tests, of course; but if the two chunks "do the same thing", think about what they have in common and try to factor it out into a single function.

Answer 8

From an OO point of view, there are two patterns that maybe appropriate in this case; Template and Strategy.

The Template pattern allows you to change the steps of your algorithm at runtime. Strategy pattern allows you to to change the algorithm.

Your pseudo code looks a bit like Python, and implementation of patterns vary depending on the idioms of the language.

Here is an example of Strategy:

class Context:
    def __init__(self, strategy):
        self.strategy = strategy
    def DoSomething(self):
        self.strategy.DoSomething();

class Strategy:
    def DoSomething(self):
        raise NotImplementedError # You have to override.

class StrategyA(Strategy):
    def DoSomething(self):
        #Large block of statements.
        print "Evaluate StrategyA"

class StrategyB(Strategy):
    def DoSomething(self):
        #another large block of statements.
        print "Evaluate StrategyB"

c = Context(StrategyA())
c.DoSomething();

c = Context(StrategyB())
c.DoSomething();

Template pattern:

class Context:
    def __init__(self, template):
        self.template = template
    def DoSomething(self):
        self.template.Step1(); # represents a difference between the two algorithms
        # common steps in here...
        self.template.Step2(); # difference...
        # another common step here...
        self.template.Step3(); # difference...
        self.template.Step4();

class Template:
    def Step1(self):
        raise NotImplementedError # You have to override.
    def Step2(self):
        raise NotImplementedError # You have to override.
    def Step3(self):
        raise NotImplementedError # You have to override.
    def Step4(self):
        raise NotImplementedError # You have to override.

class TemplateA(Template):
    def Step1(self):
        print "Step1A"
    def Step2(self):
        print "Step2A"
    def Step3(self):
        print "Step3A"
    def Step4(self):
        print "Step4A"

class TemplateB(Template):
    def Step1(self):
        print "Step1B"
    def Step2(self):
        print "Step2B"
    def Step3(self):
        print "Step3B"
    def Step4(self):
        print "Step4B"

c = Context(TemplateA())
c.DoSomething();

c = Context(TemplateB())
c.DoSomething();

However, without more context, if is difficult to tell if this is best in this situation. Both patterns maybe overkill for just two steps. It might be better just to break up your method into separate methods.