6

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?

  • 6
    Not all loops iterate over the attributes of a data structure. – Kilian Foth Jan 29 '16 at 16:30
  • @KilianFoth looping a range of integers is semantically (although not performance-wise) equivalent to mapping over an array containing those integers: coffeescript for instance provides sugar to do exactly that [0...5] is equivalent to [0,1,2,3,4]. So repeating any action n times is essentially the same as calling forEach on an array of n elements. – Jared Smith Jan 29 '16 at 16:34
  • 2
    I don't think that break and filter are interchangeable. In one case, you iterate until you encounter the first element that satisfies a predicate, in the second, you loop over all elements that satisfy a predicate. As in “cook me food on all days until Friday” versus “cook me food on all days that are work-days”. – 5gon12eder Jan 29 '16 at 16:37
  • 3
    In principle, pretty much any unbounded loop might need an explicit if(iterations > max) break; to ensure it doesn't hang the system, not just game loops. Such as interpreters which have a maximum recursion level, or linters which have a maximum number of errors they'll warn about before shutting up, etc. Kilian's original comment was basically correct. – Ixrec Jan 29 '16 at 17:01
  • 4
    map, foreach and filter don't help when the problem is "operate on all elements up to the first one that satisfies some condition." I'd go so far as to argue that using reduce for that case instead of a straight loop with a break is a code smell because it's less readable. – Blrfl Jan 31 '16 at 13:45
30

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.

  • 3
    +1, with the caveat that there are a tiny handful of exceptions, such as with. – Ixrec Jan 29 '16 at 17:15
  • 7
    “… 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.” Totally agree; I think this really hits the nail. – 5gon12eder Jan 29 '16 at 17:19
  • @Ixrec: Yeah, JS with is a remarkable abomination, a brilliant exercise in fouling up a great idea. – Nathan Tuggy Jan 29 '16 at 18:00
  • 1
    @Ixrec That's what I mean, to me a code smell means 'probably bad' whereas with is unquestionably bad. – Jared Smith Feb 1 '16 at 20:16
  • 1
    "Single keywords are not code smells. They are tools for flow control. " - N.B. This includes goto in languages that have it. – immibis Feb 3 '16 at 22:20
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.

  • As an aside, Java doesn't need the goto that C# uses to get out of multiply nested structures, because it has the labeled break and continue. Yes, they can be refactored into other calls without too much difficulty. – user40980 Feb 3 '16 at 21:44
  • could you lose your mutiple returns with a while loop plus a GetNext method – Ewan Feb 4 '16 at 9:08
  • I avoided getNext() because that is awkward when you need to return an index. And mainly, I never bothered to learn and use that Java idiom cause it didn't seem worth the effort. Just used the old "FORTRAN style" for (int i=0 ...). And apparently some high muckedy-mucks agreed, cause they added the wonderful "foreach style", ` for (T t : someCollectionOrArray);` which I do use. – user949300 Feb 6 '16 at 5:45
-6

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
      }
    }
  • 3
    "Also we should note that a function which returns from more than one place is also a code smell" - This (and your example) seems not really related to the original question anymore. – Brandin Feb 1 '16 at 10:01
  • 4
    A function with multiple returns is not necessarily a code smell. Want to see a code smell? Try six nested if statements, or the usage of a "control variable". Because that's what the "one return only" policy will create. – Hugo Zink Feb 1 '16 at 13:40
  • 5
    Downvoted due to too many opinions with too weak argumentation. And I would add: It all depends on context. Even goto is the right tool in some situations. – Torbjørn Feb 1 '16 at 15:53
  • 2
    goto is not a code smell when there are no other forms of flow control that make sense for a given case. Don't be dogmatic. Not even Dijkstra was dogmatic about it in his essay. (Have you read it?) – greyfade Feb 1 '16 at 18:05
  • 4
    Code is a code smell. Don't write code. – Blrfl Feb 1 '16 at 18:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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