3

I'm testing SonarQube in order to check the quality of my code.

In a loop, I have something like that :

foreach(var someVar in someList)
{
    if(condition)
        //Do stuff
    else
        break;
}

SonarQube tells me :

Refactor the code in order to remove this break statement.

I can do it by constructing a list which respects my condition, but can someone explain why using break is considered as a bad practice ?

t3chb0t
  • 16,340
  • 13
  • 78
  • 118
Chostakovitch
  • 965
  • 1
  • 9
  • 33
  • 1
    I use it all the time to avoid crowding my loop conditions and to avoid having to scope variables involved in the decision to outside of the loop. I'd also love to know why this is considered bad practice. – spender Jan 26 '15 at 10:47
  • 8
    Any reason not to use `TakeWhile`? That would be simpler, IMO: `foreach (var someVar in someList.TakeWhile(condition))` – Jon Skeet Jan 26 '15 at 10:47
  • @JonSkeet - nice! Didn't know about this. –  Jan 26 '15 at 10:49
  • In this particular case filtering the source before iterating would give clearer code. But "don't use `break`" in general is silly. – Jon Jan 26 '15 at 10:49
  • 2
    I'd go as far as `if (!condition) { break; }` then `//Dostuff`, which makes the break nice and obvious at the top of your loop. – Rawling Jan 26 '15 at 10:49
  • 2
    It's worth mentioning that ReSharper tends to suggest the opposite - use breaks to avoid nesting. I just judge whether it makes the code more readable on a case-by-case basis. – Theodoros Chatzigiannakis Jan 26 '15 at 10:52
  • 1
    Out of curiosity, I fed that exact message to Google and it turned up [this](https://jira.codehaus.org/browse/SONARCS-263). It says: *"The `break;` statement, as its name indicates, breaks the control flow of the program in an unstructured way. It reduces the program readability, and makes refactoring more complex when used outside of `switch`."* Can't exactly say I agree, and I'm not even sure if it's an official opinion, but it found its way into SonarQube anyway so there you go. – BoltClock Jan 26 '15 at 10:52
  • @JonSkeet No particular reason. I like your solution ; in my case, I used something like this : `var list = someUIElement.ChildrenOfType<...>().Where(condition);`, but I think it's the same. I only wanted to know if they were other reasons that *clarity* of code for not using `break`. – Chostakovitch Jan 26 '15 at 10:54
  • 1
    @Chostakovitch `Where` is *not* the same as `TakeWhile`. `Where` gets *all* the elements that match the predicate; `TakeWhile` only gets the elements that match *until the first element that doesn't match*. In other words, `While` is like replacing your `break` with a `continue`. – Rawling Jan 26 '15 at 11:32

0 Answers0