Can this architecture of base + derived classes be coded more efficient? [closed]

Posted on

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 like BuildingBuilder (or just Construction) and have a Build 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 a Action or Func<> 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.

Leave a Reply

Your email address will not be published. Required fields are marked *