Object-oriented – Extend, wrap, or both to add generics to a class that should have had them

design-patternsgenericsobject-orientedobject-oriented-design

So I'm using a C# framework that has a great example of where generics would be useful, except they weren't used. For simplicity's sake, we will say it was a list (I know C# has its own list), but instead of being List where I can specify the T, they made it List, resulting in nightmarish type checking throughout the code.

I want to fix this, but I can't edit the framework.

So I see three ways to fix this.

Extend, wrap, or both.

So the basic question is, which is the best way to add generics?

Is there a design pattern particulary for this?

Now, while I'm not sure wrapping or extending is better, if this was a simple case I would have gone with one of them and not even considered doing both.

But, the CustomList is used everywhere, and I only want to replace it bit by bit.

Extending allows me to use the TypedCustomList as a CustomList, so that if I change where CustomList are being created to instead by TypedCustomList, later in code's execution the code can still treat it like a CustomList.

But, I can't treat a CustomList object as a TypedCustomList (even if I ensure the generic type is correct for that given CustomList). (Or am I missing how to do this, which would solve the problem?)

If a wrap it, I can now work with existing CustomLists, but I have to expose the internal CustomList object to pass it on to other code (for example, if I call a method expecting CustomList, I have to do instanceOfTypedCustomList._customList, I cannot pass instanceOfTypedCustomList).

If I wrap and extend it, I get take an existing CustomList and treat it like a TypedCustomList while still being able to treat my TypedCustomList as a CustomList. But, as you can see below, the 'Both' option looks the worst.

Extending

public class TypedCustomList<T> : CustomList
{
  public TypedCustomList()
    :base()
  {
  }

  public T getElementAt(int i)
  {
    return (T) base.getElementAt(i);
  }

  public void addElement(T element)
  {
    base.addElement(element);
  }

  ...ect.
}

Wrapping:

public class TypedCustomList<T>
{
  CustomList _customList { get; private set;}
  public TypedCustomList(CustomList cl)
  {
    _customList = cl;
  }

  public T getElementAt(int i)
  {
    return (T) _customList.getElementAt(i);
  }

  public void addElement(T element)
  {
    _customList.addElement(element);
  }

  ...ect.
}

Both:

public class TypedCustomList<T> : CustomList
{
  private CustomList _customList;

  public TypedCustomList()
    :base()
  {
    _customList = null;
  }

  public TypedCustomList(CustomList cl)
  {
    _customList = cl;
  }

  public T getElementAt(int i)
  {
    if(_customList == null)
    {
      return (T) base.getElementAt(i);
    }
    else
    {
      return (T) _customList.getElementAt(i);
    }
  }

  public void addElement(T element)
  {
    if(_customList == null)
    {
      base.addElement(element);
    }
    else
    {
      _customList.addElement(element);
    }
  }

  ...ect.
}

P.S.

More background which may be relevant to the problem.

There is one class that has a CustomList that is used all over the application, with different parts of the application with each instance having different object types (over a dozen in total). I want to eventually have this class have a unique TypedCustomList for every use. But until I can be sure that all references to it's CustomList are gone, I wanted to add some error handling. So I was adding something like the following:

public ClassThatUsesCustomList
{
...
private CusotmList _list;
private TypedCustomList<SomeObject> _SomeObjectCustomList;

public CustomList list
{
    get
    {
      //Already existing code
      if(_list == null)
      {
        _list = new CustomList();
      }

      //My code
      catchMissedGetSet(_list);


      return _list;
    }
    set
    {
      catchMissedGetSet(value);
      _list = value;
    }
}

public SomeObjectCustomList
{
  get
  {
    if(_SomeObjectCustomList == null)
    {
      _SomeObjectCustomList = new TypedCustomList<SomeObject>();
    }

    return _SomeObjectCustomList;
  }
  set
  {
    _SomeObjectCustomList = value;
  }
}


private void catchMissedGetSet(CustomList cl)
{
  bool findMissedSpots = true;

  if(cl == null || cl.Count == 0 || cl[0] is SomeObject)
  {
    SomeObjectCustomList = new SomeObjectCustomList(cl);  //Using wrapping.
    if(findMissedSpots && System.Diagnostics.Debugger.IsAttached)
    {
      System.Diagnostics.Debugger.Break();  
      //Hey, you missed a place.  Use step out to find it.
    }
  }
}


...
}

I did this to catch any spots I missed and to avoid any bugs. It requires me wrapping it (as far as I can tell).

Best Answer

The answer is wrapping, but it is considerably more work than implied here. I see why you are trying to do this though, because after a certain size of the code base it indeed can be a nightmare to work with typecasting every time you call a library function. Subclassing CustomList is not advisable, as it would expose all the functions that return or expect an object. Having a public property as in the second example is, for the same reason, also unadvisable. I believe you are facing a situation similar to the following:

interface Producer
{
  CustomList produce();
}

interface Consumer
{
  void consume(CustomList list);
}

This is how I would wrap it:

public class TypedCustomList<T>
{
  private readonly CustomList _customList;
  internal CustomList customList
  {
    get { return _customList; }
  }

  internal TypedCustomList(CustomList cl)
  {
    _customList = cl;
  }

  public T getElementAt(int i)
  {
    return (T) _customList.getElementAt(i);
  }

  public void addElement(T element)
  {
    _customList.addElement(element);
  }

  ... ect.
}

The producer:

class TypedProducer<T>
{
  private readonly Producer producer = new Producer();
  public TypedCustomList<T> produce()
  {
    return new TypedCustomList<T>(producer.produce())
  }
}

The consumer:

class TypedConsumer<T>
{
  private readonly Consumer consumer = new Consumer();
  public void consume(TypedCustomList<T> list)
  {
    consumer.consume(list.customList);
  }
}

You will need to put the wrapping classes to their own assembly, so that they hide the internal parts.

I can safely suggest you this solution, which I beleive generally considered a good practice. You will need to wrap the subset of the library you are using in this manner.

Persisting objects is an orthogonal concern.

The Customer object should not know anything about how it is being saved or retrieved from a database, XML file, or whatever (the concept is called Persistence Ignorance). This is why the preferred approach, especially in a large system, is to utilize some generalized persistence mechanism like an Object Relational Mapper to store and retrieve objects, and then to draw that functionality out through Repository methods that return objects or perform some business action on the data.

This relieves you of the responsibility of having to code a persistence implementation for each and every class you create.

Further Reading
The Unit Of Work Pattern And Persistence Ignorance

Java – Designing generic type inheritance hierarchy

Type System Abuse?

Reading the post office example, there is some evidence to suggest that you might be trying to abuse the type system to encode property values in interfaces. And failing...

The way you are currently implementing it, Speed vs. Normal are mutually exclusive, while there is no reason why a class cannot simultaneously be an ExecutivePostageLot and a PublicPostageLot, as far as I can tell.

From your description, however, it appears that you want to categorize postage lots (and postages for that matter) along two dimensions: velocity (speed vs. normal) and importance (executive vs. public).

If so, having an interface for each of the values is not how you should approach it.

Alternative Interpretation

But your question and the diagrams seem to allow another way to interpret what you're trying to do. Maybe I'm imagining things, but I think you want to do the following (I'll use a different example, but it should map 1:1 to your diagram):

You have an Entity class with common methods - e.g. int Id() and DateTime Created().

You have multiple modules, each module defining its own Entity subclasses. So for example, you have a purchasing module (with entities like Order and Product), and a newsletter module (with entities like Post and Subscription).

Now, it seems you want to work with various kinds of sets of a particular subclass of Entity. Some of those sets need to provide only generic functionality that is not specific to a kind of entity. For example, there might be a SortableSet like the following:

class SortableSet<T extends Entity> 
    implements EntitySet<T>
{
    // resorts the set (i know, this makes no sense for a set).
    void Sort(IComparer<T> cmp);

    // ... whatever methods are required by EntitySet ...
    // ... these might also be abstract, of course ...
}

Each of the modules, however, may also need more specific functionality added to our sets. The newsletter module might need to combine all posts in a set to form a new super-post:

// assuming Post has subclasses, otherwise this does not
// need to be generic, of course.
interface CombinablePostSet<T extends Post>
    implements EntitySet<T>
{
    T combineAll();
}

Now we can define a class that combines the required functionality of a module with the generic functionality provided by a general purpose EntitySet, like SortableSet.

class CombinableSortablePostSet<T extends Post>
    extends SortableSet<T>
    implements CombinablePostSet<T>
{
    T combineAll() { ... }
}

If, for some reason, we want to create a class that produces a SortableSet, it should probably not create a set of basic entities, but of a specific entity type. This means we have to let the method know the type of entity that will be in the set. This can be solved with a generic method. (Or with a generic class, but let's go with the method for now).

Of course, in the method body, you can still only treat the elements as Entities, not as Products or Posts, because this method does not know anything about their module-specific functionality.

// this might be a Factory?
class CommonSortableSetLogic
{
    <T extends Entity> SortableSet<T> Processor()
    {
        SortableSet<T> set = null;
        return set;
    }
}

If instead the method returned a SortableSet<Entity> or even an EntitySet<Entity>, we cannot simply cast the returned result to just any subclass of EntitySet with any generic parameter, just because that is the type we need. This is the reason you're getting errors - your CommonMethod1EntitySetLogic methods need to provide the right kind of EntitySet.

But, factories and the like aside, the common logic for a SortableSet<T> should be implemented in the SortableSet<T> class itself. That's why its a class (from which you derive) and not an interface.

Some Notes

Assuming I have understood what you wanted to do, there's still the question of whether you should actually do it like this. We would need to know more about your actual application (what's the purpose of the Entity class - what's special about entity sets that is different from regular sets, etc).

With proper use of generics, operation classes that work on regular sets (like a SetSorter<T>) might lead to less coupling and a clearer structure, if composed well. Anyways, this is a topic for another day maybe.

If you do stick to the design, just remember that A<X> cannot be cast to B<X> or A<Y>, even if B extends A and Y extends X. This kind of thing tends to bite you when not being careful with generics.