The “Named Generic” Anti-pattern

WARNING: This is me standing on my soapbox, and if you don’t think I’m right, leave a comment. (Also, if you’re familiar with SOLID, this may seem obvious to you).

When it comes to code, I’m pretty particular about how things are named. To me, naming both classes and class members is one of the fine arts of software development. I actually think that this can demonstrate quite a bit about someone’s experience, what they understand about the software development life cycle, and ultimately, they conceptual understanding of Object-oriented design. That’s why this particular “code smell” bothers me so much.

I’ll call it the “Named Generic” pattern.

we’ve all seen it. A developer wanted to specify a typed parameter, but missed the boat.

For example:

public RecallList GetRecalls(AutoMakersList automakers)
{
// do something useful with each automaker and yield the recalls associated with them.
}

So far, this code isn’t bad. We have a decent method name, the parameter name is ok, but what are the two type definitions here:

public class AutoMakersList : List{ /*no body*/ }
public class RecallList : List{ /*no body*/ }

Uh oh.

With two classes I have simultaneously reduced flexibility, and increased complexity. From the perspective of the consumer of this method, I now have to marshal a set of strings, then add them to a new class called AutoMakersList, which I had to search out and attempt to understand.

From the API designer’s perspective, I’ve placed some requirement on what is legal to pass in. Except I haven’t. The list is still of string, and the last time I checked, there were no validation methods on string that validate they are auto maker’s names (C# 5.0, maybe?). So I’ve really just obfuscated what I wanted to happen, which was this:

“hand me an enumerable of validated automakers”

The same could be done with this method signature:

///<summary>This will produce the recalls associated with the specified automakers.</summary>
///<param name="automakers">This is a pre-validated set of automakers for which recalls
/// will be retrieved. Valid automakers follow these rules: .....
///</param>
public RecallList GetRecalls(IEnumerable automakers)
{
//iterate over each automaker, and do something interesting to yield out their recalls
}

In Visual Studio (and perhaps MonoDevelop?), I’ve now told the API consumer what I expect, they will get it intellisense when they’re constructing the call. Whereas, if I just told you to hand me an AutoMakersList, there’s ambiguity in what is required. A side note is that Code Contracts are very helpful, and you should consider using them anyway, but that’s not the point of this post. The other benefit of this approach is that I’ve reduced the calling requirements on this method. The above example is actually not done yet, and here’s where it will *seem* like I’m contradicting my point, but I’m not, really.. really.

public RecallList GetRecalls(IEnumerable automakers)
{
//do something interesting.
}

public class Automaker
{
public String Name{get; set;}
public bool IsValid(){return valid;}
}

Instead of passing in just a a list of string, why not pass in “Automaker?” On the surface, it seems very much like just passing a simple String, the difference is that I have attached the context explicitly to the Name property, instead of implicitly from the name of the collection in which the object was stored. Let that marinate in your brain for a minute. They’re actually radically different concepts, one of them works, and IMHO, one of them doesn’t.

Part two…

“RecallList”

Here, the API designer got it half right. The the context for each recall is explicitly attached to the object that cares — “Recall”, but RecallList doesn’t actually add any value, it essentially says “this is a list of Recall”, which is the same thing as what “List” says, in a much more concise way. Although I think Classes are *cheap* *cheap* *cheap* and people are too often reluctant to add them, in this case “RecallList” is just *redundant* *redundant*.

Finally, passing a list in or out adds extra overhead that neither the API designer or the caller needs. In both cases, List places extra requirements on either side of the call, when really everyone meant to say, “here’s a set of something that you can read through. (IEnumerable)” When we apply all of these suggestions together, this is the method we’re left with:

///<summary>This will produce the recalls associated with the specified automakers.</summary>
///<param name="automakers">This is a set of automakers for which recalls will be retrieved.</param>
public IEnumerable GetRecalls(IEnumerable automakers)
{

// iterate over each automaker, use the "IsValid()" method to determine if it should be processed,
// and do something interesting to yield out the recalls
}

As I said at the beginning of the post, if you’re familiar with SOLID, most of this post will make good sense to you, but I wanted to try to put it in a context of code you’ve probably seen. Let me know what you think in the comments.

Advertisement