Reflections on coding style
I've been monitoring my coding style for quite a while and I noticed that I often prefer chaining method calls to normal control flow. Now I think it might not always be such a good idea.
To give you an example, today I had a task of reflecting over constants in a class and stuffing them into a dictionary. Here's a sample to demonstrate my original approach. In this sample I replaced the actual domain model with a fake colors domain and the task is to implement two methods: FillColors and OutputColors.
using System;
using System.Collections.Generic;
static class Program
{
public const string Red = "#FF0000";
public const string Green = "#00FF00";
public const string Blue = "#0000FF";
static void Main(string[] args)
{
var dictionary = new Dictionary<string, string>();
FillColors(dictionary.Add);
OutputColors(dictionary);
}
static void FillColors(Action<string, string> add)
{
var fields = typeof(Program).GetFields();
fields.ForEach(f => add(f.Name, f.GetRawConstantValue().ToString()));
}
static void OutputColors(Dictionary<string, string> dictionary)
{
dictionary.ForEach(p => Console.WriteLine(p.Key + " = " + p.Value));
}
static void ForEach<T>(this IEnumerable<T> list, Action<T> action)
{
foreach (var item in list)
{
action(item);
}
}
}
First, several interesting comments about the code.
- constants are public because otherwise Type.GetFields won't find them. And for the life of me I couldn't figure out how the overload of GetFields works that accepts BindingFlags. I didn't want to spend more than 2 minutes on that so I just made the constants public.
- there are no more access modifiers, because members are private by default and top-level types are internal by default if the access modifier is missing. Jon Skeet wrote in his C# in Depth book that he prefers omitting the modifier if it corresponds with the default visibility and I really like this style. Makes the programs shorter. Short programs are especially useful for us testers, where we need to find a minimal repro for bugs that we report.
- FillColors accepts a delegate "add" which I like calling "the collector". It could have accepted the entire dictionary, but I only use the add functionality so to minimize coupling I only require what I actually consume. An alternative here would be to implement an iterator using yield return, but this would require returning a list of pairs of values, and Tuples were not in our TFS branch yet at the moment of writing :) Another reason for "the collector" pattern is that yield return doesn't compose well - flattening lists like Comega does is still not in C# as of 4.0. Contrary to that, passing a delegate (Continuation-Passing Style) composes nicely and is fully supported starting from C# 1.0.
- OutputColors doesn't accept IEnumerable<KeyValuePair<string, string>> because I was to lazy to spell that out and it is not relevant to the current discussion :)
- Also OutputColors doesn't accept Action<string> and hardcodes Console.WriteLine because of the KISS/YAGNI principle.
- I don't know why KISS/YAGNI didn't apply to the FillColors method. It just didn't.
But I digress. Let's go back to the actual topic of me writing one long line:
fields.ForEach(f => add(f.Name, f.GetRawConstantValue().ToString()));
instead of:
foreach (var f in fields)
{
add(f.Name, f.GetRawConstantValue().ToString());
}
Here are several observations that favor foreach against ForEach:
ForEach is not in the library
As of .NET 3.5 SP1, ForEach is not in the library so I had to implement it myself. For .NET 4.0 I've logged a suggestion with Mads to add it to the BCL, but I'm not sure if we're adding it for 4.0. A lot of people seem to add it to their libraries so I think it'll be useful to have regardless of the coding style it suggests. I'm sure it is useful at least sometimes.
ForEach hides state mutation
Eric Lippert told me that he's not a great fan of ForEach because it conceals state mutation - it takes an eager (non-lazy) control flow statement that mutates state and hides it behind a functionally-looking call.
You can't put a breakpoint inside ForEach "body"
If you use foreach, you can put a breakpoint on the add call, whereas you can't put a breakpoint on the add call in the first case. This is not a language problem, but rather a tooling issue, but still I don't see tools (yes, debugger, I'm looking at you!) becoming comfortable enough in this scenario anytime soon.
Stack is polluted
In the first case (ForEach), two additional frames are on the stack which looks kinda ugly during debugging:
vs. the original:
This may not have a good impact on the debugging story.
Performance is worse
Although I know that micro-managing performance is a bad thing to do, we still create an anonymous method (metadata bloat) and have two additional calls on the stack (I know that JIT will probably optimize them, but who knows). So the danger is that in critical codepaths (bottlenecks) the performance will degrade compared to using control flow (which the JIT will compile into a couple of nice fast JMP instructions).
Readability suffers
Readability is probably worse because ForEach is not as familiar as foreach. Also, the code lines tend to grow long with the first approach and I like my lines of code less than 80 characters.
ForEach is NOT functional style
I'd like to stress that I'm still a huge fan of functional style, immutability, composability, first-class functions, etc. The ForEach style described in this blog-post is NOT functional style, although it definitely looks like it.
- It is NOT immutable
- It is NOT idempotent
- It does not chain as Kevin mentions
- It is NOT lazy and doesn't have deferred execution like many LINQ operators which are truly functional
We had a hallway conversation with Chris Smith from F# team about this coding style topic today, and even he (the functional programming guy and a huge F# fan) admitted that sometimes C# should better use C# native constructs instead of borrowing from other (albeit awesome) languages like F#.
Summary
So I guess I'm going back to more using good old control flow statements again as they deserve it. They have been here for 30 years for a good reason and I don't think they're going away anytime soon.
