Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have to read lines from a text file in sequential order. The file is a custom text format that contains sections. If some sections are out of order, I would like to look for the starting of the next valid section and continue processing.

Currently, I have some code that looks like this:

for (int currentLineIndex=0; currentLineIndex < lines.Count; currentLineIndex++ )
{
    //Process section here
    if( out_of_order_condition )
    {
        currentLineIndex--;//Stay on the same line in the next iteration because this line may be the start of a valid section.
       continue;
    }
}

Is this code smell?

UPDATE: I didn't mention this earlier, but the root cause of this kind of code was a complicated switch-case (typical when you're parsing).

I got rid of the incrementing/decrementing variable by using the "goto case" statement.

The structure now looks like this:

switch(state)
{
   case State.BOF:
       {
           //Process BOF case
       }
   case State.SeenHeader:
       {
           if( out_of_order_condition )
           {
               state = State.BOF;    //Reset the state to some respectable one

               //currentLineIndex--; Removed
               //continue;           Removed

               goto case State.BOF;//Handle this in this iteration itself. 
           }
       }
}
share|improve this question
    
Is that code yours ? –  user61852 Dec 18 '12 at 18:00
    
@user1598390: Yeah, it's just psuedo-code. –  FairDune Dec 18 '12 at 22:24

4 Answers 4

up vote 6 down vote accepted

Well, since a code smell is something that makes you take a second look at it, which you yourself are doing, I'd say it definitely qualifies. However, code smells don't automatically need removing, just a hard look to make sure it's really the best way to solve the problem.

In this particular case, the reason you don't often see code like that is it can cause the loop to never terminate under certain input conditions. You're also commingling two different responsibilities into the loop: detecting section starts and processing a section. I would try to have one loop that only detects section boundaries, and once the entire section is known, pass the section contents to another function for processing.

It's also possibly a sign your format is complex enough that a homegrown parser is going to have difficulty catching all the boundary conditions. You might want to look into a full parser like bison or antlr.

share|improve this answer
    
Thanks. I will try these suggestions. –  FairDune Dec 18 '12 at 4:15

I think it is a code smell. The idiom in a for loop is to have it increment/decrement consistently. People reading your code are going to expect the increment expression (currentLineIndex++) to be the controlling logic.

I think it would be better to do one of the following:

  1. turn this code into a while loop. The idiom for while loops vary more making developers more likely to read your code.
  2. add a nested while loop to handle the case where you stay on one line. That way the outer loop preserves the idiom and you make it more obvious that the inner loop keeps going until out_of_order_condition is no longer met.
share|improve this answer
1  
Thanks. Looks like While is the way to go. –  FairDune Dec 18 '12 at 4:16

No. Or at least I don't think so. It is an indication that the iteration is complicated, but that's not necessarily a sign of a fundamental design problem ... or bad coding practice.

However, you might want to look at the code and think about whether there is a more readable way of doing this. And if you keep the code as it is, you might want to either:

  • add a comment at the start of the loop to say that the loop variable is changed, or

  • change this to a while loop.


On the other hand "code smell" is very subjective ...

share|improve this answer
    
Thanks. I meant general design issues. –  FairDune Dec 18 '12 at 3:46

It depends. To be on safer side on multi threaded scenario, use Interlocked.* API rather than using plain operators.

share|improve this answer
    
That's true. But my problem was more related to a possible infinite loop scenario because of variable decrementing within the "for" loop body. –  FairDune Dec 20 '12 at 3:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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