exit for = spaghetti code?

I have always been taught to have one exit from functions and write code that doesn't jump all over the place, is the following bad code or is there a better way to write the for loop without needing an "exi开发者_开发知识库t for"?

dim dt as datatable = FillDataTableFunction()
dim bWrite as boolean = false
for each row as datarow in dt.rows
  if row.item("somecolumn") <> string.empty then 
    bWrite = true
    exit for
  end if 
next

if bWrite then 
  'do stuff
end if

I guess I am just thinking that this will reduce unneeded iterations through the for loop but for some reason it seems like a bad coding practice.

"I have always been taught" - at some point in life, people start learning rather than being taught :-)

Don't take anything as gospel (even from me - if you disagree, make your own mind up). Look behind the guidelines to see why they exist.

The reason why you were taught that multiple exit points is bad is because they often lead to code that is hard to follow. In other words, a 400-line function peppered with return statements is hard to analyse in terms of its behaviour.

Your little code snippet does not suffer from this. The guideline I follow is: if you can see the control flow on a single screen in the editor, it's fine. And, since 12 lines will fit in any editor I've used in the past two decades, your code is very readable.

In fact, I've seen code from the "never use multiple exit points" people that is far less readable than that which would be produced by breaking their rules. It usually involves multi-condition while statements so convoluted that they have to break it across multiple lines, and is still a pain to analyse.

Aim for readability. If guidelines help with that, use them. If not, throw them out the window.

What you are refering to as "spaghetti code" is the use of the old keyword "goto" which was heavily used in the past. (eg: Assembly, GwBasic, etc).

Using breaks to exit loop structures and decision structures is perfectly natural.

There's not much wrong with your code snippet.

It's probably more a matter of taste but I would put this in a separate function:

Public Function ShouldWrite(ByVal dt As DataTable)
    For Each row As DataRow In dt.Rows
        If row.Item("somecolumn") <> String.Empty Then
            Return True
        End If
    Next
    Return False
End Function

Or, use LINQ:

    Dim bWrite As Boolean = dt.AsEnumerable().Any(Function(row) row.Item("somecolumn") <> String.Empty)

I understant that this was considered a bad practice too. However, I don't think it is considered that bad practice any more. In fact, I can read your code pretty easily: this is the end of well-written programs, I guess...

However, there is another step that anyone can change to enhance his/her code: In the loop, you're trying to find the first row that has an item that matches some condition. If you program more "functional" way (I don't know exactly what facilities does VB have in this regard, but it is still a good design consideration), say:

bWrite = rows.exist(r | r.item("somecolumn") <> string.empty)

Note how the whole loop is condensed in a schematic description of what you're looking for. This "way of thinking" I consider it to be much more important than if you use exit for or a complex while condition...

Having only one exit from your function is more of a guideline than a hard and fast rule - do whatever you prefer. Personally, I prefer to return things as soon as possible.

The code that you pasted looks just fine to me, and not spaghetti-like at all - you're short circuiting your loop, in order to avoid having to loop through 100,000 rows if you find your answer in the first 100. In that case, I'd say your Exit For is entirely justified.

Goto considered harmfull in modern code looks this.

def func:

  while stuff:
    ...
    return False
    ...
  end
  return true

end

In the above you are leaving the function all together out of the loop - So there are two exit points, one not where you might think it should be. The return is 'hidden' in the other lines of the loop, and may be missed by the reader.

Exit For is socially acceptable, since it effectively leaves the for loop at the bottom. The important thing is to make the code easy to understand, especially for you in a few months when you've forgotten what you did today. If you get in the habit of using Exit For, it will likely be easier to recognize at a glance than any other method of leaving the loop.

Many 'rules' of languages (whether human languages or computer languages) exist because people who had a sense of what seemed good and what seemed icky wanted to help others write well, and thus tried to identify what made 'good' stuff different from 'icky' stuff. It is not adherence (or lack thereof) to a rule which makes something good (or bad). Rather, it is the fact that things which follow the rule are (hopefully) generally better than those which do not, which led to writing the rule in the first place.

I'm not terribly keen on "Exit For"; I tend to think that in most cases where a for-next loop would early-exit, some other type of loop would have been better. Nonetheless, there are places where an early-exit For-Next loop "feels" right, and so I'll use it. I'm most apt to early-exit a for-next loop if the expectation is that the loop will run to completion, but throwing an exception for the early-exit case wouldn't feel right.

There is a lot more to consider than just if a specific command is good or bad. It depends very much on the situation.

In a simple piece of code like your example, it's probably more readable to use an exit than to try to put it in the logic of the loop. As the code that exits the loop is so close to where it's ending up, it's spaghetti-ness has such a small impact that it's outweighted by how much more complex the loop had to get to do the same thing.

If there is a lot more code, it can be hard to follow where an exit would actually end up and what effect that would have on the rest of the code, so in that case there may be a point in trying to keep a single exit point.

Instructions like exit have an inherit spaghetti tendence to them, as they make a jump to a different point in the code, but if the jump is close it will not disrupt the flow more than for example an if statement.

Another things to consider is how often a specific command is used. If a specific construct is often used, it's more likely that it will be understood.

This is the code I got to work

bool someboolean = dt.AsEnumerable().Any(row => row.Field<string>("RowName") != "")