Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
    
Can the input data for one of the cases be safely modified to match the input data for the other one? If so, I'd transform it and use one of the "large block of statements" after the transformation –  Izkata Jan 24 '13 at 19:09
    
@Izkata: That was not possible, hence the two blocks of code using if-else logic. –  veepsk Jan 25 '13 at 15:59

8 Answers 8

up vote 4 down vote accepted

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.

share|improve this answer
    
Agree with you, that is exactly what I did. –  veepsk Jan 25 '13 at 16:11

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.

share|improve this answer
1  
+1: Only thing I am not sure about is if a and b should be re-inlined anyway. If you make this method call either a or b, it is already doing one thing, it is deciding which route the code should go. Then it's the subroutines that are responsible for doing the actual work. At least my take on it. But yeah... I think he should do the obvious refactoring first, and see where it takes him. –  martiert Jan 17 '13 at 7:42
    
-1 And what are you going to do if they have everything in common. I have seen many methods that have common state that is same from beginning to end. And this makes it hard to refactor into small reusable parts. –  Euphoric Jan 24 '13 at 21:31
1  
@Euphoric, if you find yourself in that situation you should probably start thinking about using the Replace Method With Method Object refactoring. It doesn't invalidate the approach of starting by splitting out a & b. sourcemaking.com/refactoring/replace-method-with-method-object –  Carl Manaster Jan 25 '13 at 4:40

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.

share|improve this answer
    
I like this technique. Sometimes, big methods have so much state, that it makes lots of sense to create separate class or classes from single method. –  Euphoric Jan 24 '13 at 21:33

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.

share|improve this answer

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
share|improve this answer
1  
Why? What advantage does this bring? Good answers on Programmers don't just present code, they explain the reasoning behind it. –  Walter Jan 17 '13 at 12:30
    
good point. I have updated the post –  rikAtee Jan 17 '13 at 14:40

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.

share|improve this answer

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
share|improve this answer

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.

share|improve this answer
    
Thank you for your answer, well I cannot use the way you told because the method is itself inside another class. –  veepsk Jan 25 '13 at 16:05
    
Yes, you can. There is a basic refactoring, "Sprout Class", see Working Effectively with legacy code or alternatively this one sourcemaking.com/refactoring/replace-method-with-method-object –  Dave Hillier Jan 28 '13 at 22:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.