I have a small break in coding. I’m enjoying a recently written LINQ projecting list to another list 🙂 The first list represents rows from a simple table. The second contains objects grouping rows according to the requirements. I’m waiting because I know that for a minute this code will be replaced by simpler ForEach. That’s because it contains .ToDictionary().
The code snippet above shows this projection. As one can see, flat list of ATMTaskTypePriorityEntry objects is grouped by TaskTypeDbId property and each group is converted to ATMTaskTypePriorities object whose constructor accepts TaskTypeDbId and Dictionary<int,Enum> object.
This approach works well in almost every situation but has some flaws. In my opinion, these flaws are disqualifying.
Flaws:
Complexity
This projection is not very complex, but even though it’s not obvious what it does. Reader, in a not so distant future, needs to stop for a while and think about it. As we know, code is much often read than written, so we have to take care to make it as readable as possible. There is a simple fix for this issue though. Enclose the LINQ in a method with a name explaining what it does. Then the code will look like this:
Works well in standard conditions
This flaw is a gamechanger. The previous one could be fixed easily, but probably there is no fix for this one. What am I talking about?
ToDictionary() converts a list of table rows to a dictionary. As we know, a dictionary has to have unique keys. If EventThreshold, which is the key, will be duplicated, then ArgumentException occurs.
Of course, we will try to prevent it. We can add constraints to the table, validate user input and so on. This will work like a charm for a long time when the sun is shining and clients are happy. But then, someday, and that day may never come, I will call upon you to do a service for me… I mean – someday you will obtain a ticket from support informing you about unexpected crashes on the customer’s site. And then you will spend hours or days trying to figure out what’s going on, never even suspecting this small ToDictionary thing. Because the code is protected as hell, right?
No. The code WAS protected. But then customer care removed some constraints from the table, as the performance was not sufficient. Or due to something else, nevermind. The point is, that in the future the application will be fed with wrong data and the code will crash.
Deal with it.
The solution is not so sexy as long, complex LINQ proving that you’re mastah programma. It’s standard, poor foreach construct that will convert your beautiful but fragile Alfa Romeo to ugly but steady and predictable Skoda.
Please remember that Pastebin makes the code uglier that it’s in reality 🙂 Anyway, I combined two ideas. I removed ToDictionary from the code and enclosed projection in a separate method.
The code is maybe longer, but very obvious. Additionally, we can rearrange it as we want, add auxiliary items (for example inner ForEach can contain logging in case of something strange happen) and we’re sure that the code will not throw.
Conclusion
In conclusion, this article is not meant to repel anyone from using ToDictionary() extension. As always, before implementing something one has to think about the possible consequences of choosing specific constructs. I think that rare cases like „duplicated keys from constrained data on user’s site” may be rare enough not to worry about them. But in my job exactly such cases decide whether the code is really good. It’s not about how „sexy” it is, because the client is usually straight and not attracted by code itself, rather by how it works.