Casting stinks. Generic classes are worse.

Posted by Matthew Watkins on April 12, 2018

Let’s go on an imaginary journey in class design for a minute.

Suppose that you have the following classes representing vehicles. You go the polymorphic approach and either write them to inherit from a common base class or implement a common interface:

public abstract class Vehicle
{
  public void Move() { }
  public abstract int PassengerCapacity { get; }
  // ...
}

public class Sedan : Vehicle
{
  public override int PassengerCapacity => 4;
  public bool IsConvertible { get; set; }
  public int TireSize { get; set; }
  // ...
}

// Other classes for motorcycle, truck, boat, etc.

So far so good, but these are complicated objects and require a factory to build them. And they vary enough that each one requires its own typed custom factory, assembly line, assembly line configuration, and engineers. So you create base classes or interfaces for those entities:

public abstract class VehicleAssemblyLineInstructions
{
  public int MaxNumberOfUnitsToBuildInParallel { get; set; }
  // ...
}

public abstract class VehicleEngineer
{
  public string Name { get; set; }
  // ...
}

public abstract class VehicleAssemblyLine
{
  public VehicleEngineer LeadEngineer { get; set; }
  public abstract Vehicle Build(VehicleAssemblyLineInstructions instructions);
}

public abstract class VehicleFactory
{
  public string FactoryName { get; set; }
  public VehicleAssemblyLine AssemblyLine { get; set; }
  // ...
}

Now that the base classes or interfaces exist, you move forward and create concrete implementations:

public class SedanAssemblyLineInstructions : VehicleAssemblyLineInstructions
{
  public int AirbagSafetyRating { get; set; }
  // ...
}

public class SedanEngineer : VehicleEngineer
{
  public bool IsCertifiedByFord { get; set; }
  // ...
}

public class SedanAssemblyLine : VehicleAssemblyLine
{
  public string SedanMakersUnionType { get; set; }
  public abstract Vehicle Build(VehicleAssemblyLineInstructions instructions)
  {
    // TODO: Implementaton here
    throw new NotImplementedException();
  }
}

public class SedanFactory : VehicleFactory
{
  public string SedanManufacturerName { get; set; }
}

OK so here’s where we run into our first problem. In your concrete implementation’s build method, in order to access sedan-specific members of the assembly instructions or the engineer, you need to cast them to the specific type. And who knows if they will cast or not. The compiler is not enforcing that they pass what you want:

public class SedanAssemblyLine : VehicleAssemblyLine
{
  public abstract Vehicle Build(VehicleAssemblyLineInstructions instructions)
  {
    // The compiler enforces that I have a VehicleAssemblyLineInstructions object. No idea whether the caller knew my
    // inner workings enough to guess that I really want a SedanAssemblyLineInstructions. Guess we'll find out now!
    SedanAssemblyLineInstructions sedanAssemblyInstructions = (SedanAssemblyLineInstructions)instructions;
    SedanEngineer sedanEngineer = (SedanEngineer)this.LeadEngineer;
    if (sedanAssemblyInstructions.AirbagSafetyRating < MIN_SAFETY && sedanEngineer.IsCertifiedByFord)
    {
      // ...
    }
  }
}

Yuck. And then after all that work, we return back the abstract type representation. So even if the caller magically knew to pass in the concrete input types, they have to magically know to cast the output types (and pray that we returned the right one):

SedanFactory sedanFactory = new SedanFactory
{
  AssemblyLine = new SedanAssemblyLine
  {
    SedanMakersUnionType = "Honda Sedan Workers of America",
    LeadEngineer = new SedanEngineer
    {
      Name = "Bob Jones",
      IsCertifiedByFord = true
    }
  }
};

SedanAssemblyLineIstructions sedanAssemblyLineIstructions = new SedanAssemblyLineIstructions
{
  MaxNumberOfUnitsToBuildInParallel = 2,
  AirbagSafetyRating = 4
};

Vehicle vehicle = sedanFactory.AssemblyLine.Build(sedanAssemblyLineIstructions);
Sedan sedan = (Sedan)vehicle;

Console.WriteLine($"Is convertible? {sedan.IsConvertible}. Tire size: {sedan.TireSize}");

So. much. Casting. Can we get rid of it? Well, yes, but it’s not pretty. Here are the base classes:

public abstract class Vehicle
{
  public void Move() { }
  public abstract int PassengerCapacity { get; }
  // ...
}

public abstract class VehicleEngineer
{
  public string Name { get; set; }
  // ...
}

public abstract class VehicleAssemblyLineInstructions
{
  public int MaxNumberOfUnitsToBuildInParallel { get; set; }
  // ...
}

public abstract class VehicleAssemblyLine<E, I, V>
 where E : Engineer
 where I : VehicleAssemblyLineInstructions
 where V : Vehicle
{
  public E LeadEngineer { get; set; }
  public abstract V Build(I instructions);
}

public abstract class VehicleFactory<L, E, I, V>
  where L : VehicleAssemblyLine<E, I, V>
  where E : Engineer
  where I : VehicleAssemblyLineInstructions
  where V : Vehicle
{
  public string FactoryName { get; set; }
  public L AssemblyLine { get; set; }
  // ...
}

And here are the new concrete classes:

public class Sedan : Vehicle
{
  public override int PassengerCapacity => 4;
  public bool IsConvertible { get; set; }
  public int TireSize { get; set; }
  // ...
}

public class SedanEngineer : VehicleEngineer
{
  public bool IsCertifiedByFord { get; set; }
  // ...
}

public class SedanAssemblyLineInstructions : VehicleAssemblyLineInstructions
{
  public int AirbagSafetyRating { get; set; }
  // ...
}

public class SedanAssemblyLine : VehicleAssemblyLine<SedanEngineer, SedanAssemblyLineInstructions, Sedan>
{
  public string SedanMakersUnionType { get; set; }
  public abstract Vehicle Build(SedanAssemblyLineInstructions instructions)
  {
    // Look ma, no casting!
    if (instructions.AirbagSafetyRating < MIN_SAFETY && LeadEngineer.IsCertifiedByFord)
    {
      // ...
    }
  }
}

public class SedanFactory : VehicleFactory<SedanAssemblyLine, SedanEngineer, SedanAssemblyLineInstructions, Sedan>
{
  public string SedanManufacturerName { get; set; }
}

There are a ton of benefits to this on our end. All the method arguments, properties, etc are strongly-typed to the generic constraints by the compiler. The caller can’t pass anything to us that is not the exact type we want, and we don’t have to cast a thing. Same goes for the caller code:

SedanFactory sedanFactory = new SedanFactory
{
  AssemblyLine = new SedanAssemblyLine
  {
    SedanMakersUnionType = "Honda Sedan Workers of America",
    LeadEngineer = new SedanEngineer
    {
      Name = "Bob Jones",
      IsCertifiedByFord = true
    }
  }
};

SedanAssemblyLineIstructions sedanAssemblyLineIstructions = new SedanAssemblyLineIstructions
{
  MaxNumberOfUnitsToBuildInParallel = 2,
  AirbagSafetyRating = 4
};

// The sedan factory's assembly line returns a sedan. Just like I want!
Sedan sedan = sedanFactory.AssemblyLine.Build(sedanAssemblyLineIstructions);
Console.WriteLine($"Is convertible? {sedan.IsConvertible}. Tire size: {sedan.TireSize}");

Much nicer on the consumer end. But wow, that VehicleAssemblyLine class with 4 generic parameters makes me sick. Also, the generic arguments make it really hard to handle in an abstract way. For example, say you previously had a method that took in an arbitrary factory and printed its name:

public void Print(VehicleFactory factory)
{
  Console.WriteLine($"Welcome to {factory.FactoryName}");
}

Not possible anymore. Now that one method gets really ugly:

public void Print<L, E, I V>(VehicleFactory<L, E, I, V> factory)
  where L : VehicleAssemblyLine<E, I, V>
  where E : Engineer
  where I : VehicleAssemblyLineInstructions
  where V : Vehicle
{
  Console.WriteLine($"Welcome to {factory.FactoryName}");
}

One more thing: let’s say you were serializing your objects somewhere in XML, JSON, or YML. And let’s say you want to read a collection of them, but you don’t know or care about the specific types in this case– just that you want to read the common properties. You remove the abstract keyword from those classes so the parser can construct them. Without the generics, you could just do this:

VehicleFactory factory = JsonConvert.DeserializeObject<Factory>(payload);
Console.WriteLine($"Factory {factory.FactoryName}'s assembly line is run by {factory.AssemblyLine.LeadEngineer.Name}");

But no more. Now, it gets really, really hairy:

VehicleFactory<VehicleAssemblyLine<Engineer, VehicleAssemblyLineInstructions, Vehicle>, Engineer, VehicleAssemblyLineInstructions, Vehicle> factory = JsonConvert.DeserializeObject<VehicleFactory<VehicleAssemblyLine<Engineer, VehicleAssemblyLineInstructions, Vehicle>, Engineer, VehicleAssemblyLineInstructions, Vehicle>>(payload);
Console.WriteLine($"Factory {factory.FactoryName}'s assembly line is run by {factory.AssemblyLine.LeadEngineer.Name}");

Even if you’re one of the “cool kids” who use the var keyword for all your variable declarations, you still end up wrapping lines just to declare a type.

There are other issues with the generic approach, like how there is compiler weirdness that doesn’t seem to handle generic inheritance quite right, but in my opinion, the biggest issue is the fact that the number of generics in your class declaration will grow linearly with the number of concrete properties you want to expose through the generic type declarations. And while it will always start out with only one or two properties, it will always grow.

But on the other hand, the alternative to growing your generic type arguments is growing the number of annoying and dangerous casts you and the caller both have to perform.

I just don’t see a nice way to do this. Is there some other design pattern or practice that I am forgetting about that will solve this? Comments are really appreciated.

This post first appeared on Another Dev Blog