TL;DR; A lot of your if-statements are unnecessary and makes you violate the open/closed principle (open for extension, closed for modification) among other things. Make sure to identify those and get rid of them a soon as possible. They are the ones which checks a type or parameter and then decide what to do in that case.
Open/closed principle
The O in SOLID principles. What it actually means is that a method and in extension, a class, should not be open for modification but only extension. The exception being if the rules actually have changed. Following this principle makes you and your fellow coders avoid changing the behavior by accident.
You could regard this as an extension of SRP (Single Responsibility Principle), where a specific piece of code have one and only one purpose. And the open/closed principle forces you even further in that direction, aswell as making your code DRY and encourages reuse and readability.
An ugly example
While it is easy to think of this for green field projects, it is probably even more useful in brownfield applications in the progressive work of cleaning up and make heads and tails of things. A not uncommon scenario is some kind of Init() method which takes care of some in data validation and checks for a type or enum to then decide what to do.
public class MyInitClass
{
public void Init (string accomodationType) {
//do some stuff
switch (accomodationType) {
case "apartment":
//do stuff
CalculateApartmentFare();
//do stuff
break;
case "house":
//do stuff
CalculateHouseFare();
//do stuff
break;
case "room":
//do stuff
CalculateRoomFare();
//do stuff
break;
default: throw new ArgumentOutOfRangeException("Invalid type provided.");
}
//do even more stuff
}
}
It looks innocent and perhaps not that intimidating. But what if the contents weren’t in methods? Even worse, there might be twenty accomodation types. And you can count on that there will be more in the future. You will have to make modifications to this piece of code each time, which might be cumbersome depending on what it looks like aswell as adding a bit of risk of not changing anything else. Who knows what might be hidden underneith or inbetween? There is also the impending doom of this if-statement being copied all over and not knowing all places to change.
The remedy
Don’t fear, there is something fairly easy you could do in several steps. One small baby step at a time. If the contents aren’t in methods. Do that first, make sure to stream line it so they behave as expected for each type and does only one thing.
After that perhaps you could break out the entire switch-statement into a method of it’s own. Is it copied else-where? Put the brand new method in a class and point all of the others to that class as well.
Still not satisfied, but coming closer. Even though it is now isolated in a class of its own, there is this ugly switch-statement. Not a fun thing to update each time and it is bound to the fare calculations of all the accomodation types. Break away the calculations to new classes for each accomodation type. Since all of the methods being called are of the type Calculate(), make them implement and interface with that single method on it.
Now it’s time to get rid of the pesky switch-statement, the foundation is there. Put the type-names in a dictionary with their corresponding calculation class as the value. Since they inherit the same FareCaluculator interface you can call Calculate() on all of them! It is more loosely coupled and you can safely add new ones without fear of disturbing others.
public class MyInitClass
{
private readonly AccomodationCalculationConditioner _accomodationCalculationConditioner;
public MyInitClass()
{
_accomodationCalculationConditioner = new AccomodationCalculationConditioner();
}
public void Init(string accomodationType)
{
var calculator = _accomodationCalculationConditioner.GetCalculationConditionForType(accomodationType);
var fare = calculator.Calculate();
//do more stuff
}
}
public class AccomodationCalculationConditioner
{
private readonly Dictionary _conditions;
public AccomodationCalculationConditioner()
{
_conditions = new Dictionary
{
{"apartment", new ApartmentCalculator()},
{"house", new HouseCalculator()},
{"room", new RoomCalculator()}
};
}
public IAccomodationsFareConditions GetCalculationConditionForType(string accomodationType)
{
if (_conditions.ContainsKey(accomodationType))
{
return _conditions[accomodationType];
}
throw new ArgumentOutOfRangeException("accomodationType", "Invalid accomodation type provided");
}
}
public class RoomCalculator : IAccomodationsFareCalculation
{
public double Calculate()
{
throw new System.NotImplementedException();
}
}
public class HouseCalculator : IAccomodationsFareCalculation
{
public double Calculate()
{
throw new System.NotImplementedException();
}
}
public class ApartmentCalculator : IAccomodationsFareCalculation
{
public double Calculate()
{
throw new System.NotImplementedException();
}
}
public interface IAccomodationsFareCalculation
{
double Calculate();
}
Now that wasn’t so hard was it? And even more, almost all of the SOLID principles are in there or are at least possible to apply if the need arises. But these are just bonuses, what it all boils down to and is the important point here, is if you consider the understandability of the Init() method. Is it hard to read? Is it hard to understand? Can you figure out what it does fairly easily? How about testability? And, would it be hard to change something in there? Would you consider it riskful to change something and not knowing what else might get affected? This is called orthogonality, that is something good.
Wrap up
Now, this is not _the_ way, it is only _a_ way. Which other ways do you know of? How do you approach dragons and other beasts out there luring in the code jungle? Hit me up on twitter to let me know!
Further reading