Lance's Whiteboard

Why prefer using for? I was under the impression that foreach was more safe to use.

Excellent question.

This is a recommendation that I should have given more details on in the document. Let me explain both sides of the argument.

Using "foreach" is often considered safe to use because it will always call Dispose() on the IEnumerator upon completion.

However, it does this at the cost of additional performance overhead due to the IEnumerator implementation.

Also, you can't modify a collection while you're enumerating over it.

This isnt a problem if you need just read-only access, but by getting into the habbit of managing your own iterations, you will be less likely to require changes in your code when you realize you DO need to modify the collection.

I still use foreach statements for small or trivial loops where it makes sense, but as a whole, I advocate developers to "prefer 'for' over 'foreach'"

nicely summarized code standards doc. you saved me a lot of work; i can just point to your pdf and call it a day!

That's a horrible excuse. foreach should be preferred over for in almost every situation. Foreach is an implementation of the iterator design pattern, and properly decouples the structure from the iteration. foreach allows you to swap out the back end datastructure without every having to rewrite the loop, thus iterating over a collection is the same as iterating over a tree. Unless you are doing 3d graphics I doubt you could ever justify a situation where the performance difference between for and foreach was even a factor. Using a for does nothing but introduce a chance for bugs that foreach prevents. You are suffering from premature optimization, foreach is far superior to for.

Mr. Anon (65.211.217.3),

I appreciate your feedback. As I mentioned earlier, I do plan to review and clarify the reasons and caveats to this standard in the next version.

Having said that, I must disagree with your assertion that "foreach should be preferred over for in almost every situation". Although performance is a real and valid reason for preferring "for", the primary reason to include the For/Foreach rule was an attempt to avoid common pitfalls caused by developers performing updates within a foreach statement. I have performed countless code reviews where exceptions and unexpected behaviors have occurred due to the many abuses of foreach.

This is exactly the type of behavior a good coding standard tries to prevent. But, the current rule is probably a bit more black & white than it should be. After receiving several comments on this rule, its obvious that this rule should be improved.

My current plan is to split it into multiple related rules governing the use of foreach. The end result should be less severe than the original statement to "prefer for over foreach" and more of an emphasis upon avoiding "foreach" under circumstances that commonly lead to problems.

I look forward to your feedback on version 1.12 of the standards document once it is released in the next few weeks.

Thank you, and keep the feedback coming...

The simplest way to update a collection in a single loop is to simply Clone in in the loop, i.e.

foreach(Customer aCustomer in CustomerCollection.Clone())
if(aCustomer.IsExpired)
CustomerCollection.Remove(aCustomer);

I've yet to see a scenario in a typical business application where the difference in performance between for and foreach is a valid factor. If a developer doesn't know you can't modify a collection while iterating it, the developer is the problem, not the foreach. For is an archaic low level construct that shouldn't even be in the language, and is far more error prone than foreach. Once anonymous delegates come out next year, there won't even be an excuse to ever write a loop, as collections should simply include higher order functions to do the loop for you.

CustomerCollection.Remove(delegate(aCustomer){return aCustomer.IsExpired;});

Predicates and higher order functions are the proper way to handle all iteration. I agree with many of the thing in your standards, but I just can't swallow this one, I think you are simply wrong and too accustomed to old habbits.

You make a compelling argument. I am not sure I had ever tried the Clone approach, and had merely fallen-back to using "For" instead of working through the issue.

Let me chew on this a bit more...its always nice to learn new tricks.

Thanks again for your feedback!

I also disagree with putting braces on a new line. What a waste of space. I understand the motivation, you want to see matching curly braces to easily define scope. But there's a better way, ignore the braces and look at indention, that's why we like

if(condition){
some code...
}else if(condition){
some other code
}else if(condition){
some other code
}

vs

if(condition)
{
some code...
}
else if(condition)
{
some other code
}
else if(condition)
{
some other code
}

It's cleaner, fits more of the function onto the screen, and scope is easily viewed by looking at indention rather than braces. This usually can reduce line count by hundreds of lines in some cases, and as weird as it sounds, it helps make the code look more like a machine by keeping it in a tight block and your eyes can pick it out better than if it's all spaced out with a zillion line breaks for every }. I understand this is a religious issue, but there really are advantages to the K&R style, it is better and leads to cleaner nicer looking code.

OK, your board pulled out all the spaces for indention in the above code, is there some way to make the code look proper here?

I also disagree with using braces on all conditional statements. If you use the guard clause idiom, this isn't acceptable. There are too many cases where a method start's off with this..

if(somecondition)return;

or

if(somecondition)return bla;
else if(othercondition)return ble;

//rest of method

Simply put, use {} only when you need them, and I find most of the time, I don't ever need them. If I have a bunch of conditional code, it usually looks like this

if(condition)Method1();
else if(othercondition)Method2();
else if(othercondition)Method3();
else if(othercondition)Method4();
else if(othercondition)Method5();

because it's cleaner to seperate the implementation of each block into seperate methods, allowing one to cleanly see the overal logic, and rearrange it without messing with the implementation. Not that I always do this, but it is often the case and requiring {} is pointless and makes the code ugly.

The K&R vs newline method for curly-braces is always a holy war. I stated as much in the document, but felt that the absence of a standard for braces would lead to too much inconsistency.

My opinion, of course is exactly the opposite. I find K&R far less readable, and I have never seen any need for reducing LOC in this day and age.

Having said this, your later comment about the rule "Always use curly-braces in Conditional statements" does ring true. I have debated this topic many times, and the frequent violation of this rule in my own code shows that I do not belive it 100%. I would like to change that rule to merely discourage the if/else behavior you showed in the latter example. This is the main reason this rule was created in the first place.

In most cases, the repeating if/else statements truly should be changed to a switch statement. However, there are definitely alot of caveats to what is or is-not appropriate in these cases.

I appreciate your comments and feedback. They will definitely be considered in the next draft.

Note: sorry about the missing spaces in the comments, its a "feature" of the .TEXT blogging engine that the http://weblogs.asp.net site uses.

Thanks,

Lance

Note:

I'd like to hear an explanation of how K&R is less readable to you, if you're so inclined, I totally disagree, but then I don't look at the braces for scope, but I do enjoy hearing other opinions.

Also, I disagree that in most cases a repeating if else should be a switch. Repeating if else's are for more complex conditions that aren't a single value that can be switched on. If it were simple enough to switch upon, that usually calls for classes instead of switch statements.

In most cases a switch should be a group of classes and polymorphism should be used instead. Good object oriented code is almost completely devoid of switch statements, except possibly in factories where they can choose the correct class.

Code that has switch statements scattered around, is almost always bad code. Switch statements foster redundant code and should be considered a serious smell to be closely scrutinized. If polymorphism can be used instead, then it should be.

K&R:

There are plenty of newsgroup threads & blogs on this topic, but here is my take.

Much like using black text on white background (or vice versa) to give contrast, Whitespace around code makes it more readable. K&R is a style that leads to much more condensed code which, IMHO, leads to excessive clutter in the code-window. The newline style is the opposite. It injects an additional level of whitespace (beyond that of indentions)that makes it much more easily read.

I follow this style for the same reasons that UI-design guru Jakob Nielson evangelizes removing clutter from web UI's. Its kinder on the eyes, and easier to comprehend with a mere glance.

As an added bonus, I have found K&R problematic in codegen while newline style does not. However, not having used K&R as much, I won't guarantee I have tried every angle with K&R and codegen.


Switch statements:

I have read the comments countless times about the offensiveness of Switch statements and their propensity for Code Smell. I understand and agree with some of the arguments, but do not agree with throwing-out a perfectly good tool because it is often abused. Switch statements don't kill OO, people kill OO!

For example; there is ABSOLUTELY NO DIFFERENCE between using a switch statement and using if/else in your example above. They are both equally offensive, and foster the same level of redundant code;

if(condition)Method1();
else if(othercondition)Method2();
else if(othercondition)Method3();
else if(othercondition)Method4();
else if(othercondition)Method5();


switch(condition)
{
case value1:
Method1();
break;
case value2:
Method2();
break;
case value3:
Method3();
break;
case value4:
Method4();
break;
case value5:
Method5();
break;
default:
break;
}

Granted, you can argue against the switch statement for aesthetic reasons, but regarding your argument on OO-offending traps, both the nested-if/else and switch statements are equally offensive.

Don't get me wrong, switch statements should definitely be limited to simple conditions only, such as evaluating against an Enum or other known sets of values. But, I cannot toss-out this tool just because alot of people use it instead of polymorphism.

However, this conversation does bring to mind a number of new rules I should consider. I may add something about Conditionals vs. Polymorphism to the OO guidelines section of the doc.

You're wrong about the if else....

if else's are for complex conditions where each condition is checking different things

if(condition)Method1();
else if(a&&b||c)Method2();
else if(c&&d&&f)Method3();
else if(o&&(f&&a))Method4();

something switches can't do, any condition simple enough for a switch statement can be turned into polymorphism if (a big if) you have access to the source of the class that could hold the polymorphic method.

I also think you are wrong about the whitespace making code more readable... that statement tells me you're doing something like this. Ignore _'s

void Method(arg)
{
____if(conditions)
____{
________SomeCode();
________SomeCode();
________SomeCode();
____}

____if(otherconditions)
____{
________SomeMoreCode();
________SomeMoreCode();
________SomeMoreCode();
____}
}

now that may seem clean and clear... but to me, it looks like badly factored code.... using white space to seperate logical groups of statements is an antipattern.... the language already provides a far better construct to set off a logical group of statements... the method. That code could would be more clearly written as....

void Method(arg)
{
____if(conditions)SomeMethod();
____if(otherconditions)SomeOtherMethod();
}

void SomeMethod(){
____SomeCode();
____SomeCode();
____SomeCode();
}

void SomeOtherMethod(){
____SomeMoreCode();
____SomeMoreCode();
____SomeMoreCode();
}

and if you need whitespace to seperate logical blocks, then put a space between each method. Now K&R comes in handy, it makes the blocks cohesive and makes each method look like an individual chunk. More importantly, we are using the language properly, all logical groups of statements want to be methods... that's what a method is for. Methods in properly factored code rarely have more than a few lines of code and always fit on one screen, and have no need for whitespace to break up the logic. As an additional bonus, each individual method is now easier to reason about, change, fix, ect... without affecting the main logic or possibly breaking it.