Problem
I’ve got some architecture in my code similar to this. It is about the Construct method in the base class in the example below. This method contains the logic for constructing a building, which is the same in every building. Yet different buildings may want to do things differently, like a different roof. So I’ve used events to do this. Is this a valid architecture? I know it works, but is there a better way? I want to leave the logic of constructing any building in the base class, and not override it to just build another roof and copying 66% of its code.
Remember this is an example. In the code I had to create, it not uses 3 functions in the Construct base method as the logic flow, but over a dozen.
Base class
public abstract class Building
{
/// <summary>
/// use this event in the derived class to lay another foundation.
/// </summary>
public event EventHandler OnFoundation;
/// <summary>
/// Use this event in the derived class to build different walls.
/// </summary>
public event EventHandler OnWalls;
/// <summary>
/// Use this event in the derived class to build a different roof.
/// </summary>
public event EventHandler OnRoof;
/// <summary>
/// The number of floors a building as determines how high it will be.
/// </summary>
public readonly int NumberOfFloors;
/// <summary>
/// Constructor.
/// </summary>
/// <param name="argNumberOfFloors">The number of floors a building as determines how high it will be.</param>
protected Building(int argNumberOfFloors)
{
NumberOfFloors = argNumberOfFloors;
}
public void Construct()
{
//Lay the foundation.
if (OnFoundation != null)
{
OnFoundation(this, new EventArgs());
}
else
{
Foundation();
}
//Build the walls
if (OnWalls != null)
{
OnWalls(this, new EventArgs());
}
else
{
Walls();
}
//Build the roof
if (OnRoof != null)
{
OnRoof(this, new EventArgs());
}
else
{
Roof();
}
}
public void Foundation()
{
//Lay foundation
}
public void Walls()
{
for (int i = 0; i < NumberOfFloors; i++)
{
//Build walls
}
}
public void Roof()
{
//Build roof
}
}
Implementations
Villa
public class Villa : Building
{
public Villa() : base(2)
{
base.OnRoof += Villa_OnRoof;
}
/// <summary>
/// A villa gets another roof, the rest stays the same
/// </summary>
void Villa_OnRoof(object sender, EventArgs e)
{
//Build another roof than the base class would.
}
}
SkyScraper
public class SkyScraper : Building
{
public SkyScraper() : base(60)
{
base.OnFoundation += SkyScraper_OnFoundation;
}
void SkyScraper_OnFoundation(object sender, EventArgs e)
{
//A skycraper needs a better foundation.
}
}
Solution
This works but is very unusual. The standard way of doing this would actually be virtual methods
public abstract class building {
...
protected virtual CreateWalls() {
//create 4 walls by default
}
protected virtual CreateFoundation() {
//straightforward foundation by default
}
public void Construct() {
CreateWalls();
CreateFoundation();
}
}
public class SkyScraper : Building {
public override CreateFoundation() {
//create deep foundation in bedrock
}
}
While this is the simplest and is certainly acceptable architecture it is always a good idea to also consider Composition over Inheritance
So instead you might do something like
public abstract class Building {
protected Building(WallConstruction walls, FoundationConstruction foundations, ...) {
walls.ConstructFor(this);
foundations.ConstructFor(this);
}
}
or even more generic
public abstract class Building {
protected Building(IBuildingComponent[] components) {
components.ForEach(x => x.ConstructFor(this));
}
}
a couple more things to consider.
- Does it make sense for a
Building
to be in an unconstructed state? Should this instead be something likeBuildingBuilder
(or justConstruction
) and have aBuild
method that produces a building? - The whole
(EventArgs, object)
pattern for events really hasn’t made much sense since we got good delegate signatures in .Net 3.5. Just make the signature for the event exactly what you want it to be and use aAction
orFunc<>
type. -
When using events, instantiate the event to an empty one
public event Action foo = delegate {};
that way the delegate will never be empty so there will be no need for a null check.