Matthew Watkins

Posted on Apr 12, 2018 • Originally published at anotherdevblog.net

Casting stinks. Generic classes are worse.

#csharp #design

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.

Top comments (9)

Sam Ferree • Apr 13 '18 • Edited

***Sorry for clicking "Edit Post" like 20 times, but I kept thinking of more ways interfaces really help here.

Code to interfaces, not concrete implementations?

public interface IEngineer
{
  //Engineer Properties
}

public class LeadEngineer : IEngineer
{
  //Lead Implementation
}

public interface IAssemblyLineInstructions { }
public class SedanAssemblyLineInstructions : IAssemblyLineInstructions {}
public interface IVehicle {}
public interface IAssemblyLine {}

public class AssemblyLine
{
  private IVehicle _vehicle;
  private IEngineer _enginee;
  private IAssembyLineInstructions _instructions
}

public interface IFactory 
{
  public IAssemblyLine { get; }
}

public interface INamedFactory : IFactory
{
  string FactoryName { get; }
}

public abstract class VehicleFactory : INamedFactory
{
  public string FactoryName { get; set; }
  public IAssemblyLine AssemblyLine { get; set; }
}

public void Print(INamedFactory factory)
{
  Console.WriteLine($"Welcome to {factory.FactoryName}");
  Console.WriteLine($"The Engineer on duy is {factory.AssemblyLine.Engineer.Name}");
}

public class SedanAssemblyLine : IAssemblyLine
{
  //Assembly lines have engineers
  public IEngineer Engineer { get; set; }
  public SedanAssemblyLine()
  {
    // Set defaults in the constructor, not using generics.
    // This assembly lines engineer is a SedanEngineer
    Engineer = new SedanEngineer();
  }
}

if you have functionality that you're wrapping, that's fine too.
define an abstract class that contains common functionality, and make sure it implements the interface, then just subclass it and set your custom implementations in the constructors of base classes

public interface IFactory
{
  public IAssemblyLine AssemblyLine { get; }
  public void PrintEngineer();
}

public abstract class Factory : IFactory
{
  public IAssemblyLine AssemblyLine { get; set; }
  //All factories will do this, in the same way, so Don't Repeat Yourself
  public void PrintEngineer()
  {
    Console.WriteLine("The engineer is ${AssemblyLine.Engineer.Name}");
  }
}

public class SedanFactory : Factory
{
  public SedanFactory()
  {
    //Sedan Factories have Sedan Assembly Lines... Obviously....
    AssemblyLine = new SedanAssemblyLine();
  }
}

rrconstantin • Apr 17 '18

That is exactly how I would approach the problem. This approach is a combination of SOLID principles (interface segregation and dependency inversion mostly) and Bridge pattern (decouple an abstraction from its implementation so that the two can vary independently). Also,

"if you have functionality that you're wrapping, that's fine too.",

we can use Template pattern here.

Bob van Hoove • Apr 13 '18 • Edited

Perhaps I'm glossing over things.. correct me if I am:

If I look at this:

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}");
}

This looks like you want to host a variety of L, E, I and V instances. But the thing that really varies is the kind of Sedan.

If you abstract the thing that's similar yet varies, you'll end up with a less elaborate type signature and the class could look roughly like this:

public Factory<ISedan>
{
    public AssemblyLine<ISedan> assemblyLine; 

    // the lead comes with the team
    public EngineeringTeam<ISedan> engineers; 

    public ISedan Create(Instruction<ISedan> instruction) 
    { /* ... */ }    
}

Scott Hannen • Apr 13 '18 • Edited

This answer is going to be a little bit fuzzy and vague. One suggestion is to start by writing a single implementation end-to-end without using inheritance, then another, and then apply the inheritance if and when it makes sense. Starting with inheritance often paints us into a corner.

In the case of a class like this:

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

Inheriting from VehicleAssemblyLine suggests polymorphism. But if the class is going to be cast as its base type then where will the SedanMakersUnionType fit in?

Also, we typically use a factory to create a class when we need it. That means that in any given scenario we need one instance of a specific class. If we're trying to create numerous instances of multiple objects of varying types, what needs them?

I realize it's just an example. But perhaps what I'm trying to say is that it's better if we start from something simple that achieves one instance of exactly what we need when we need it. This sort of problem happens when we work the other way, trying to create a "generic" solution for a requirement that hasn't been clearly defined.

I understand because I've gotten to the same point many, many times.

I hope the title of this post - The Generic Rabbit Hole of Madness - doesn't seem derogatory, because that's not the intent. But this seems like a perfect case of that. I wrote the post because I've done it so many times.

Matthew Watkins • Apr 16 '18

Yeah I saw that post come out a few days after mine and immediately added it to my Pocket list-- thanks! :)

Harvey Thompson • Apr 14 '18 • Edited

I know this is in C#, but if this were general question for some abstract language then the problem might be solved with what Scala calls Abstract Types (I think it's better to call them Abstract Member Types).

If you just want to solve it in C#, read no further in my post, because this is of no help (sorry about that).

Otherwise see:

The idea is to move the types from generic parameters to abstract (member) types:

public abstract class VehicleAssemblyLine
{
  public abstract type E : Engineer;                    // Totally made up C# syntax, WILL NOT COMPILE
  public abstract type I : VehicleAssemblyLineInstructions;
  public abstract type V : Vehicle; 

  public E LeadEngineer { get; set; }
  public abstract V Build(I instructions);
}

What this means is that sub-classes of VehicleAssemblyLine have to provide three type definitions of E, I and V that must sub-class described.

This at first does not seem to be that much different than providing them as generic arguments, but when you sub-class it makes more sense:

public class Sedan : Vehicle
{
  public override type E = SedanEngineer; // C# made up syntax, IT NO COMPILE
  public override type I = SedanAssemblyLineInstructions;
  public override type V = Sedan;
   ...
}

The idea is that Sedan provides a more concrete definition of type E, so all the inherited methods of Sedan from Vehicle return the more specific type. It essentially moves the parameterization from the declaration site into the body. This reduces all the generic arguments and reduces exponential growth of generic parameters.

For example, you can still refer to a VehicleFactory without all the generic arguments, and if required you can do safe casting to SedanFactory or others as required.

(If this were C++ you might be able to use type traits to simulate all of this).

I'm not sure what other languages have abstract member types? Possibly Swift's associated types are a similar thing?

Perhaps C# and other languages will adopt this feature in later releases. Until then simulating it in C# means using generics and lots of casting and typing. Ugh.

Deni • Apr 13 '18

I've actually spent a lot of time thinking about problems like this and there are some rules of thumb I use when I get stuck on some architecture design.

Before typing any code (or thinking about architecture design) make sure you fully understand the problem you're trying to solve and how much that problem can vary. If you don't understand the problem then you can't model the solution right (for instance maybe you think penguins aren't birds because they can't fly). If you don't have a vision on how much a problem can vary you won't know how much flexibility you require. You may overcomplicate.
When not sure how to model a solution try to brute force it. Start typing some code and notice which pieces are appearing over and over and then simply collect those pieces into an abstraction. This way, a good model for your problem will be a bit more clear. You need to make sure abstractions contain only code common to all concrete implementations. This advice sounds silly but it's not always easy to tell what's common and what's specific and sometimes you don't notice you made mistake in that regard.

My advice is to take a step back from this issue. It's imaginary. For instance it's hard to tell do you really require a class for both assembly line and assembly line instructions? Do you need to pass instructions to the assembly line as a parameter (maybe one instruction applies to only one assembly line)? What is a lead engineer class? Is that just data or does it have some complicated methods? Too many foggy questions and ofcourse we will have a hard time thinking of a proper model for this solution.

One more advice and this one sort of comes from 1. and 2. I would suggest you design your models from concrete implementations to their abstractions. This way you won't find yourself in a situation where you require a switch case cast hell. Ofcourse to model like this, rule 1. already needs to be covered.

For the end just a short note. For now I never needed to use generics in my architecture designs. There was always a better alternative (even a small cast can be better). Still generics are awesome when it comes to data structures but how often do you type those?