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.
}
}
}
2
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.
0
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:
- turn this code into a while loop. The idiom for while loops vary more making developers more likely to read your code.
- 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.
0
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
whileloop.
On the other hand “code smell” is very subjective …
0
It depends. To be on safer side on multi threaded scenario, use Interlocked.* API rather than using plain operators.
1