Is break a code smell?

Question

I'm asking in terms of a loop, obviously break is important in switch statements. Whether or not switch statements themselves are code smells is a separate issue.

So consider the following use cases for iterating a data structure:

1. You want to do something to the entire structure (no break needed)
2. You want to do something to part of a data structure.
3. You want to find something(s) in the data structure (which may or may not involve iterating the entire structure)

The above list seems more-or-less exhaustive to me, maybe I'm missing something there.

Case 1 can be thrown right out, we can use map/forEach. Case 2 sounds like filter or reduce would work. For case 3, needing to iterate the data structure to find something seems plain wrong, either the data structure itself should provide a relevant method or you are likely using the wrong data structure.

While not every javascript data structure (or implementation) has those methods it's trivially simple to write the relevant functions for pretty much any javascript data structure.

I saw this when researching but it is explicitly in the context of C/C++. I could understand how it would be more-or-less a necessity in C, but my understanding is that C++ has lambdas and many data structures are generally objects (e.g. std::vector, std::map), so I'm not sure I understand why the answers are all so universally in favor of break there, but I don't feel I know C++ well enough to even begin to comment.

I also realize that for some corner-case exceedingly large data structure the cost of iterating the entire structure may be unacceptably high, but I doubt those are very common when working even in node.js. Certainly it's the kind of thing you'd want to profile.

So I just don't see a use-case for break in today's javascript. Am I missing something here?

Answer 1

Having a break out of a loop is no different than having that loop get refactored out to a function of its own and a return statement in a guard clause.

while(condition) {
  if(test) { break; }
  doStuff;
}

vs

doMuchStuff();

function doMuchStuff() {
  while(condition) {
    if(test) { return; }
    doStuff;
  }
}

Those are effectively the same.

Single keywords are not code smells. They are tools for flow control. Many of them are variations on goto wrapped with some other level of safety to avoid the nightmare of spaghetti code.

In judging if a particular bit of code is problematic, it is necessary to look at how it works and if it is appropriate for the language constructs. The key word alone is not sufficient to establish a code smell. It might be a whiff in that it can be misused, but going on witch hunts because one sees a switch or break or some other flow control construct is counterproductive and leads to style guides that prevent people from writing understandable, straight forward code.

Answer 2

@MichaelT asked how to rewrite the C# example code for goto without using goto. Here is their code:

using System;
class Test
{
   static void Main(string[] args) {
      string[,] table = {
         {"Red", "Blue", "Green"},
         {"Monday", "Wednesday", "Friday"}
      };
      foreach (string str in args) {
         int row, colm;
         for (row = 0; row <= 1; ++row)
            for (colm = 0; colm <= 2; ++colm)
               if (str == table[row,colm])
                   goto done;
         Console.WriteLine("{0} not found", str);
         continue;
   done:
         Console.WriteLine("Found {0} at [{1}][{2}]", str, row, colm);
      }
   }
}

The first thing you notice is that, like much example/demo code, it prints the result. Any demo code that prints the result is B.S. Since hardly any real function ever prints the result. It should return a result, modify an Object, append to a Stream, etc...

(Minor sidetrack...) In this case, no big deal, but try "demo" Node.js code where you are deep inside a 3rd nested asynchronous callback, wondering how the heck that result is going to get used, and the demo just ignores the problem by printing something. Grrr. :-(

But, that gets you thinking. That code is printing, and it is doing more than one thing. Now, I'm less fanatic than many about Single Responsibility Principle, but, if you are so stuck in the muck that a goto is looking good, you are violating SRP.

Gee, what if I refactor, and write a general purpose function to search in a 2D array? Here's the initial rewrite: Note - I'm not a C# programmer, so this is in Java.

public class Test {

    public static void Main(String[] args) {
          String[][] table = {
             {"Red", "Blue", "Green"},
             {"Monday", "Wednesday", "Friday"}
          };
          for (String str : args) {
             int[] found = indexOf2D(table, str);
             if (found == null)
                 System.out.println(str + " not found");
             else
                 System.out.println(str + " found at " + found);
       }
    }


    public static int[] indexOf2D(Object[][] array, Object grail) {
        for (int r = 0; r<array.length; r++) {
            Object[] row = array[r];
            for (int c = 0; c<row.length; c++)
                if (grail.equals(row[c]))
                    return new int[] {r,c};
        }
        return null;
    }
}

Now, in a real program, you'd refactor again, writing a general purpose "search in a 1D array" function. And you'd test for null inputs. You'd discuss returning null vs. a "Null Object" vs. an Optional. You end up with code that is a skillion times more versatile, more robust, and is cleanly factored.

And the code doesn't use a freaking goto. As said before, If your code is so complex that using a goto is required, your code is too complex.

"Oh, but you've added two more method calls and an object creation, that will slow things down". Bah. If this is only getting called occasionally, the few extra nanoseconds don't matter. And, if this code is time critical, maybe you should reconsider using an O(N) linear search through your 2D data structure. :-)

A comment on why break/continue are "more smelly" than return.

Returns are easier for the programmer to reason about. You get out of the method, the stack and any local variables go away. In effect, any "closure-like" information for that method vanishes and you don't have to worry about it. (er, unless you opened up resources like files that need to be closed...)

With a break or continue, you jump forward in the code, but there is still local state to remember. That can be hard. Which local variables are still in scope? What is the value at the end of the loop? The programmer reading the code most hold the "closure" of that state in their head.

Closures are useful and cool, but better, in general, for the compiler or interpreter to hold all that state, not my feeble mind.

Answer 3

Yes, it is a 'code smell'.

This harks back to the days of spagettii code, gotos, sosubs and breaking out of loops to carry on with a procedure when ine of several conditions had been reached.

These practices have almost universaly been replaced with OO style methods, while, foreach and inhertiance. So if you still have break in your code you should ask yourself if you need it.

Obviously though 'code smell' is just an indicator that something might be wrong. Not a hard rule which can never be broken.

Also we should note that a function which returns from more than one place is also a code smell.

eg:

function doMuchStuff() {
  while(condition) {
    if(test) { return; }
    doStuff;
  }
}

should be

function doMuchStuff() {
  while(condition && !test) {
    doStuff;
  }
}

this is to avoid the kind of nightmare you can get into with

function doMuchStuff() {
  while(condition) {
    if(test) { return; }
    doStuff;
    if(test2) { return stuff; }
    doStuff2;
    if(test3) { return differentStuff; }
    doStuff3;
...etc
      }
    }