5

Is it possible to avoid code duplication in such cases? (Java code)

void f()
{
    int r;
    boolean condition = true;
    while(condition)
    {
        // some code here (1)

        r = check();
        if(r == 0)
            break ;
        else if(r == 1)
            return ;
        else if(r == 2)
            continue ;
        else if(r == 3)
            condition = false;

        // some code here (2)

        r = check();
        if(r == 0)
            break ;
        else if(r == 1)
            return ;
        else if(r == 2)
            continue ;
        else if(r == 3)
            condition = false;

        // some code here (3)
    }
    // some code here (4)
}

int check()
{
    // check a condition and return something
}

A possible solution may be using Exceptions, but that doesn't seem to be a good practice. Is there any so-called good pattern of program flow control in such cases? For example, a way to call break ; from inside the check() function. (Possibly in other programming languages)

Jonas
  • 121,568
  • 97
  • 310
  • 388
semekh
  • 3,867
  • 2
  • 24
  • 34
  • Maybe you can create a recursive function and call it back once when r equals 3. – Kevin Jan 27 '12 at 12:29
  • What is the difference between the return and the break cases? Do you have some code after the while loop? – ShaMan-H_Fel Jan 27 '12 at 12:43
  • @ShaMan-H_Fel: Yes, I do. But that doesn't make great difference to the question. – semekh Jan 27 '12 at 12:53
  • @Pf94 Although some people love recursive functions, I would NOT recommend them to replace straightforward code like the example above. Debugging becomes very nasty when using recursion. – Hidde Jan 27 '12 at 12:56

6 Answers6

4

Some good answers (especially @Garrett's just now) to a tough question but I'll add my $0.02 for posterity.

There is no easy answer here about how to refactor this block without seeing the actual code but my reaction to it is that it needs to be redesigned.

For example, a way to call break ; from inside the check() function. (Possibly in other programming languages)

If you are asking for a different break that Java does not support (without a hack) and having the duplicated check() and various different loop exit/repeat code indicates to me that this is a large and complicated method. Here are some ideas for you to think about:

  • Each of the some code here blocks are doing something. If you pull those out to their own methods, how does that change the loop?

  • Maybe break the loop down into a series of comments. Don't get deep into the code but think about it conceptually to see if a different configuration drops out.

  • Have you had another developer in your organization who is not involved with this code take a look at it? If you explain in detail how the code works someone they may see some patterns that you are not since you are in the weeds.

I also think that @aix's idea of a finite state machine is a good one but I've needed to use this sort of mechanism very few times in my programming journeys -- mostly during pattern recognition. I suspect that a redesign of the code with smaller code blocks pulled into methods will be enough to improve the code.

If you do want to implement the state machine here are some more details. You could have a loop that was only running a single switch statement that called methods. Each method would return the next value for the switch. This doesn't match your code completely but something like:

int state = 0;
WHILE: while(true) {
    switch (state) {
       case 0:
            // 1st some code here
            state = 1;
            break;
       case 1:
            state = check();
            break;
       case 2:
            return;
       case 3:
            break WHILE;
       case 4:
            // 2nd some code
            state = 1;
            break;
        ...
    }
 }

Hope some of this helps and best of luck.

Gray
  • 115,027
  • 24
  • 293
  • 354
  • 2
    +1 for comprehensive answer. Although I suspect the state-based logic can be refactored out entirely when you see all he is doing is control flow (break, return, continue) in his states. – Garrett Hall Jan 27 '12 at 15:01
  • The state machine concept via switch is nicely written down. Note that switching back to `state` 1 after "2nd some code" in case 4 is not equivalent to the code in the question, since in the subsequent while-loop, "3rd some code" needs to be executed instead. So you would need to somehow encode into the state whether you are in the first or second case distinction block. Using an additional boolean variable which only gets checked where necessary, without adding further switch-cases, looks best to me. – DaveFar Jan 27 '12 at 17:17
  • Agreed. It wasn't supposed to be perfect. The point was to show the design but really it was to encourage a redesign of the code. – Gray Jan 27 '12 at 17:21
2

The best way to avoid this duplication is not to let it happen in the first place by keeping your methods small and focused.

If the // some code here blocks are not independent, then you need to post all the code before someone can help you refactor it. If they are independent then there are ways to refactor it.

Garrett Hall
  • 29,524
  • 10
  • 61
  • 76
1

Code smell

First of all, I second aix's answer: rewrite your code! For this, the state design pattern might help. I would also say that using break, continue and return in such a way is just as much a code smell as the code duplication itself.

Having said that, here is a solution, just for fun

private int r;
void f()
{
    distinction({void => codeBlock1()}, {void => codeBlock4()}, {void => f()}, 
      {void => distinction( {void => codeBlock2()},{void => codeBlock4()},
                            {void => f()}, {void => codeBlock3()} )
      });
}

void distinction( {void=>void} startingBlock, {void=>void} r0Block, {void=>void} r2Block, {void=>void} r3Block){ 
        startingBlock.invoke();
        r = check();
        if(r == 0)
            r0Block.invoke();
        else if(r == 1)
            {}
        else if(r == 2)
            r2Block.invoke(); 
        else if(r == 3)
            // if condition might be changed in some codeBlock, you still
            // would need the variable condition and set it to false here.
            r3Block.invoke();
}

This uses closures. Of course the parameters r0Block and r2Block could be ommited and instead codeBlock4() and f() hard-coded within distinction(). But then distinction() would only be usable by f(). With Java <=7, you would need to use an Interface with the method invoke() instead, with the 4 implementations codeBlock1 to codeBlock4. Of course this approach is not at all readable, but so general that it would work for any business logic within the codeBlocks and even any break/return/continue-orgy.

DaveFar
  • 7,078
  • 4
  • 50
  • 90
0

Not really. The second continue is redundant (your code would continue anyway). Try using the Switch statement. It will make your code more readable.

Alex Calugarescu
  • 838
  • 5
  • 9
0

One nicer way to do it would be to use switch statements, something like this:

void f()
{
int r;
boolean condition = true;
while(condition)
{
outerloop:


    r = check();
    switch(r){

    case 0: break outerloop;

    case 1: return;

    case 2: continue;

    case 3: condition  = false;


}
DukeOfMarmalade
  • 2,680
  • 10
  • 41
  • 70
0

You might want to think about re-formulating your logic as a state machine. It might simplify things, and will probably make the logic easier to follow.

NPE
  • 486,780
  • 108
  • 951
  • 1,012