Refactoring away labeled loops.

Question

After I was convinced that labeled breaks/continues are a total "nono" over here, I need help to remove the label out of my code.

I have a square matrix and a vector that has the same length. The vector has already some values in it an depending on the values in the matrix the vector is changed in the loop.

I hope, the code-fragment is basically understandable

vectorLoop:
for( int idx = 0; idx < vectorLength; idx++) {
    if( conditionAtVectorPosition( v, idx ) ) continue vectorLoop;

    matrixLoop:
    for( rowIdx = 0; rowIdx < n; rowIdx++ ) {
        if( anotherConditionAtVector( v, rowIdx ) ) continue matrixLoop;
        if( conditionAtMatrixRowCol( m, rowIdx, idx ) ) continue vectorLoop;
    }
    setValueInVector( v, idx );
}

please convince me, that there is a more readable/better version without the labels.

Answer 1

Easily, my good man.

for( int idx = 0; idx < vectorLength; idx++) {
  if( conditionAtVectorPosition( v, idx ) ) continue;

  for( rowIdx = 0; rowIdx < n; rowIdx++ ) {
    if( anotherConditionAtVector( v, rowIdx ) ) continue;
    if( conditionAtMatrixRowCol( m, rowIdx, idx ) ) break;
  }
  if( !conditionAtMatrixRowCol( m, rowIdx, idx ) )
    setValueInVector( v, idx );
}

EDIT: Quite correct you are Anders. I've edited my solution to take that into account as well.

Answer 2

Does this work for you? I extracted the inner loop into a method CheckedEntireMatrix (you can name it better than me) - Also my java is a bit rusty.. but I think it gets the message across

for( int idx = 0; idx < vectorLength; idx++) {
    if( conditionAtVectorPosition( v, idx ) 
    || !CheckedEntireMatrix(v)) continue;

    setValueInVector( v, idx );
}

private bool CheckedEntireMatrix(Vector v)
{
    for( rowIdx = 0; rowIdx < n; rowIdx++ ) {
        if( anotherConditionAtVector( v, rowIdx ) ) continue;
        if( conditionAtMatrixRowCol( m, rowIdx, idx ) ) return false;
    }   
    return true;
}

Answer 3

Gishu has the right idea :

for( int idx = 0; idx < vectorLength; idx++) {
    if (!conditionAtVectorPosition( v, idx ) 
        && checkedRow(v, idx))
         setValueInVector( v, idx );
}

private boolean checkedRow(Vector v, int idx) {
    for( rowIdx = 0; rowIdx < n; rowIdx++ ) {
        if( anotherConditionAtVector( v, rowIdx ) ) continue;
        if( conditionAtMatrixRowCol( m, rowIdx, idx ) ) return false;
    }  
    return true;
}

Answer 4

I'm not too sure to understand the first continue. I would copy Gishu and write something like ( sorry if there are some mistakes ) :

for( int idx = 0; idx < vectorLength; idx++) {
    if( !conditionAtVectorPosition( v, idx ) && CheckedEntireMatrix(v))
        setValueInVector( v, idx );
}

inline bool CheckedEntireMatrix(Vector v) {
    for(rowIdx = 0; rowIdx < n; rowIdx++)
        if ( !anotherConditionAtVector(v,rowIdx) && conditionAtMatrixRowCol(m,rowIdx,idx) ) 
            return false;
    return true;
}

Answer 5

From reading your code.

• I noticed your eliminating the invalid vector positions at conditionAtVectorPosition then you remove the invalid rows at anotherConditionAtVector.
• It seems that checking rows at anotherConditionAtVector is redundant since whatever the value of idx is, anotherConditionAtVector only depends on the row index (assuming anotherConditionAtVector has no side effects).

So you can do this:

• Get the valid positions first using conditionAtVectorPosition (these are the valid columns).
• Then get the valid rows using anotherConditionAtVector.
• Finally, use conditionAtMatrixRowCol using the valid columns and rows.

I hope this helps.

Answer 6

Looking at the solutions presented so far:

• They all look less readable than the original, in that they involve spending more code on the mechanism of the code rather than on the algorithm itself

• Some of them are broken, or were before they were edited. Most damning is the fact that people are having to think quite hard about how to write the code without labels and not break anything.

• Some come with a performance penalty of running the same test twice, which may not always be trivial. The alternative to that is storing and passing round booleans, which gets ugly.

• Refactoring the relevant part of the code into a method is effectively a no-op: it rearranges how the code is laid out in the file, but has no effect on how it's executed.

All of which makes me believe that, at least in the case of this question as phrased, the label is the correct solution and doesn't need to be refactored away. Certainly there are cases where labels are used incorrectly and should be refactored away. I just don't think it should be treated as some unbreakable rule.

Answer 7

@Sadie:

They all look less readable than the original, in that they involve spending more code on the mechanism of the code rather than on the algorithm itself

Externalizing the second loop outside the algorithm is not necessarily less readable. If the method name is well chosen, it can improve readability.

Some of them are broken, or were before they were edited. Most damning is the fact that people are having to think quite hard about how to write the code without labels and not break anything.

I have a different point of view: some of them are broken because it is hard to figure out the behavior of the original algorithm.

Some come with a performance penalty of running the same test twice, which may not always be trivial. The alternative to that is storing and passing round booleans, which gets ugly.

The performance penalty is minor. However I agree that running a test twice is not a nice solution.

Refactoring the relevant part of the code into a method is effectively a no-op: it rearranges how the code is laid out in the file, but has no effect on how it's executed.

I don't see the point. Yep, it doesn't change the behavior, like... refactoring?

Certainly there are cases where labels are used incorrectly and should be refactored away. I just don't think it should be treated as some unbreakable rule.

I totally agree. But as you have pointed out, some of us have difficulties while refactoring this example. Even if the initial example is readable, it is hard to maintain.

Answer 8

@Nicolas

Some of them are broken, or were before they were edited. Most damning is the fact that people are having to think quite hard about how to write the code without labels and not break anything.

I have a different point of view: some of them are broken because it is hard to figure out the behavior of the original algorithm.

I realise that it's subjective, but I don't have any trouble reading the original algorithm. It's shorter and clearer than the proposed replacements.

What all the refactorings in this thread do is emulate the behaviour of a label using other language features - as if you were porting the code to a language that didn't have labels.

Answer 9

@Sadie:

I don't have any trouble reading the original algorithm. It's shorter and clearer than the proposed replacements.

I don't have any readability trouble either. My trouble here is more philosophical: in the initial algorithm, Mo reasons negatively: if some conditions occur don't do something. I'd rather think positively: if some conditions occurs do something. "The less negations the less likely it ends up wrong."

Answer 10

This question was not about optimizing the algorithm - but thanks anyway ;-)

At the time I wrote it, I considered the labeled continue as a readable solution.

I asked SO a question about the convention (having the label in all caps or not) for labels in Java.

Basically every answer told me "do not use them - there is always a better way! refactor!". So I posted this question to ask for a more readable (and therefore better?) solution.

Until now, I am not completely convinced by the alternatives presented so far.

Please don't get me wrong. Labels are evil most of the time.

But in my case, the conditional tests are pretty simple and the algorithm is taken from a mathematical paper and therefore very likely to not change in the near future. So I prefer having all the relevant parts visible at once instead of having to scroll to another method named something like checkMatrixAtRow(x).

Especially at more complex mathematical algorithms, I find it pretty hard to find "good" function-names - but I guess that is yet another question

Answer 11

I think that labelled loops are so uncommon that you can pick whatever method of labelling works for you - what you have there makes your intentions with the continues perfectly clear.

---

After leading the charge to suggest refactoring the loops in the original question and now seeing the code in question, I think you've got a very readable loop there.

What I had imagined was a very different chunk of code - putting the actual example up, I can see it is much cleaner than I had thought.

My apologies for the misunderstanding.