Friday, June 5, 2009

When member overload backfires

I do believe that quality of software design isn’t based solely on appropriate high level decisions. It is detailed quality decisions made on a smaller scale that make it possible.

The API influences consuming code. Making it obvious and simple helps to avoid creating hard to read and write error-prone code.

Recently I caught myslef writing the API presented below (I fixed it, honestly :-)):

// Represents basic document template - named placeholders can be mapped to data and
// resulting document is generated
public class DocumentTemplate
{
       // Maps named placeholder to data item
       public void Map<T>(string placeholderName, T data)
       {
              // Placeholder will be replaced with textual representation of the data
       }

       // Maps named placeholder to collection of data items - this is a repeatable placeholder
       public void Map<T>(string placeholderName, IEnumerable<T> data)
       {
              // Placeholder is treated as a repeatable piece
              // Data collection will be enumerated and for each item placeholder body will
              // duplicated and replaced with textual representation of the item
       }

       // ... Other non-relevant members are omitted
}

Consider code snippet below – it uses type inference. Can you tell which overload will be called?

TemplateMapping templateMapping = ...
var list = new List<int> {1, 2};
templateMapping.Map("v1", list); // #1
templateMapping.Map("v2", list.AsEnumerable()); // #2

It does require you to know overload resolution rules to figure out which overload will be called. In #1 case Map<List<int>>(…) will be called and Map<int>(…) in #2.

But worse it can lead to hidden bugs. Suppose we have a method like this

static IEnumerable<int> GetCollection()
{
       return new List<int> { 1, 2 };
}

And it is used with buggy-prone templating API

TemplateMapping templateMapping = ...
templateMapping.Map("v", GetCollection());

In this case Map<int>(…) is called. But suppose that the author of the GetCollection() method decided to change return type to IList<int> - other overload will be called!

It should be clear from looking at the calling code which overloaded member will be called.

In order to solve this you can for example specify manually which overload must be used

templateMapping.Map<int>("v", GetCollection());

But wait. This API has a more fundamental problem. Overloaded members are meant to be semantically equivalent. But in this particular case mapping semantic changes based on parameters types! Latter example shows case where your application may behave differently.

This problem can be solved through giving each semantically different member unique descriptive name. For example,

// Maps named placeholder to data item
public void MapSingle<T>(string placeholderName, T data)

// Maps named placeholder to collection of data items - this is a repeatable placeholder
public void MapEnumerable<T>(string placeholderName, IEnumerable<T> data)

This will also allow to avoid overload resolution problems mentioned above.

Summary:

  • AVOID making overloads that are semantically different
  • DO NOT require from developers deep knowledge of overload resolution rules in order to use your API

1 comment:

xenn_33 said...

I assume that there is a typo in first code snippet: class name should be "TemplateMapping" :)

All in all, many times I asked myself about why in Java there is no overload methods and where are the advantages of such approach. One is here.