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

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>

/// <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.
}
}


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.

• I totally agree, except for your use of ForEach instead of foreach and delegate {} instead of () => {}. The latter being far less important, as it is simply a matter of style. The former for it's impurity. – Magus Apr 21 '14 at 22:03
• I've heard the argument about ForEach but I don't see how it really holds water. Just don't affect any inputs. Any purity of other statements like Select is solely by convention anyways. – George Mauer Apr 22 '14 at 4:11
• It's mostly about how unlike every other Linq statement, ForEach does not take and return a list. Select may have side effects, but it returns something that can be consumed. ForEach can only terminate a sequence of operations, and because it can only be used in such a different way and does nothing that the inbuilt foreach cannot, it's use is generally discouraged. It's more about the return than anything else, really. – Magus Apr 22 '14 at 14:22
• All depends on how you implement ForEach :) I usually have it return the original list so I can keep chaining - it's basically an "enumerate with side-effects" valve. But it's is a legitimate matter of style - I wouldn't reject a PR with foreach or anything. – George Mauer Apr 22 '14 at 15:00
• @GeorgeMauer Glad I asked, and glad you answered. How could I have mist the virtual/override mechanism! Thanks for the refresher! – Mike de Klerk Apr 22 '14 at 16:26