The "Named Generic" Anti-pattern

06 Jun 2010

Part One: 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, their 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<String>
{ 
	/*no body*/ 
}

public class RecallList : List<String>
{
	/*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<String> 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 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. 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 seemlike I’m contradicting my point, but I’m not, really.. really.

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

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

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. 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<Recall> GetRecalls(IEnumerable<Automaker> 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
}

Hopefully the above snippet makes some sense and shows why we should fight the urge to add classes that don’t bring their own “flavor” to the application.


comments powered by Disqus