CodeSOD: Gotta Get 'Em All

LINQ brings functional programming and loads of syntactic sugar to .NET languages. It's a nice feature, although as James points out, it helps if your fellow developers have even the slightest clue about what they're doing.

// some validation checkingvar retrieveDocIdList = this.storedDocumentManager.GetAllForClientNotRetrieved(client.Id).Select(x => x.Id.ToString(CultureInfo.InvariantCulture)).ToList();retrieveDocIdList.ForEach(id => { var storedDoc = this.storedDocumentManager.Get(int.Parse(id))// do some other stuff with the doc});

James writes:

The code snippet is somewhat paraphrased because the actual code is poorly formatted and full of junk, but this is the main point.
It seems to be a requirement that previous developers leave weird code behind with no documentation or comments explaining what they were thinking at the time.

Well, first, "poorly formatted and full of junk" is our stock-in-trade, but we do appreciate the focus on the main WTF. Let's see if we can piece together what the developers were thinking.

If you've read an article here before, your eyes almost certainly will catch the x.Id.ToString and the int.Parse(id) calls. Right off the bat, you know something's fishy. But let's walk through it.

this.storedDocumentManager.GetAllForClientNotRetrieved(client.Id) returns a list of all the documents that have not beeen loaded from the database. Now, by default, this is equivalent to a SELECT *, so instead of getting all that data, they pick off just the IDs as a string in the Select call.

Now, they have a list of IDs of documents that they don't have loaded. So now, they can take each ID, and in a ForEach call" fetch the entire document from the database.

Well, that's what it does, but what were they thinking? We may never know, but at a guess, someone knew that "Select star bad, select specific fields", and then they just applied that knowledge without any further thought. The other possibility is that the team of developers wrote individual lines without talking to anyone else, and then just kinda mashed it together without understanding how it worked.

James replaced the entire thing:

foreach (var storedDoc in this.storedDocumentManager.GetAllForClientNotRetrieved(client.Id)) { //do some other stuff with the doc}

[Advertisement] Incrementally adopt DevOps best practices with BuildMaster, ProGet and Otter, creating a robust, secure, scalable, and reliable DevOps toolchain.