I am trying to implement some of the principles laid out in Clean Code by Robert C. Martin.
I had a class that was heavily suffering from the ordering problem. I have solved most of this by extracting the code into separate classes.
I am now left with my main class looking like this:
public class Planner {
public EstimateCollection ChosenEstimates { get; set; } // Want a better name for this..
public PlannedYear YearPlan { get; set; }
public Planner(List<Estimate> estimates) {
ChosenEstimates = new EstimateCollection(estimates);
YearPlan = ReadExistingPlan();
}
private PlannedYear ReadExistingPlan() {
var planReader = new PlanReader();
var rawPlan = planReader.GetScheduleFromDatabase(ChosenEstimates.StartDate, ChosenEstimates.EndDate);
var planConverter = new PlanConverter();
return planConverter.ConvertRowsToPlannedYear(rawPlan);
}
}
// This class was made by extracting most of what used to be in Planner's constructor
public class EstimateCollection {
public List<Estimate> Estimates { get; set; }
public DateTime StartDate { get; }
public DateTime EndDate { get; }
public EstimateCollection(List<Estimate> estimates) {
Estimates = estimates;
StartDate = GetEarliestStartDate();
EndDate = GetLatestEndDate();
}
private DateTime GetEarliestStartDate() {
// Do something to get start date from Estimates list
}
private DateTime GetLatestEndDate() {
// Do something to get end date Estimates list
}
}
The problem I have now is on in the constructor (line #8) in the Planner class. First the ChosenEstimates variable is set, then the YearPlan variable is set. The ChosenEstimates var must be set before YearPlan can be set via the ReadExistingPlan() method because ReadExistingPlan() uses ChosenEstimates.
I could supply ChosenEstimates to ReadExistingPlan() as a parameter, but the Clean Code examples almost never do this.
Are there further structural changes to my code that can avoid this?
Right now I am choosing which problem I would rather have.
4
I could supply ChosenEstimates to ReadExistingPlan() as a parameter, but the Clean Code examples almost never do this.
Actually Clean Code does exactly this on page 303:
G31: Hidden Temporal Couplings
Temporal couplings are often necessary, but you should not hide the coupling. >Structure the
arguments of your functions such that the order in which they should
be called is obvious. Consider the following: General 303<code>public class MoogDiver {Gradient gradient;List<Spline> splines;public void dive(String reason) {saturateGradient();reticulateSplines();diveForMoog(reason);}...}</code><code>public class MoogDiver { Gradient gradient; List<Spline> splines; public void dive(String reason) { saturateGradient(); reticulateSplines(); diveForMoog(reason); } ... } </code>public class MoogDiver { Gradient gradient; List<Spline> splines; public void dive(String reason) { saturateGradient(); reticulateSplines(); diveForMoog(reason); } ... }The order of the three functions is important. You must saturate the
gradient before you can reticulate the splines, and only then can you
dive for the moog. Unfortunately, the code does not enforce this
temporal coupling. Another programmer could call reticulateSplines
before saturateGradient was called, leading to an
UnsaturatedGradientException. A better solution is:<code>public class MoogDiver {Gradient gradient;List<Spline> splines;public void dive(String reason) {Gradient gradient = saturateGradient();List<Spline> splines = reticulateSplines(gradient);diveForMoog(splines, reason);}...}</code><code>public class MoogDiver { Gradient gradient; List<Spline> splines; public void dive(String reason) { Gradient gradient = saturateGradient(); List<Spline> splines = reticulateSplines(gradient); diveForMoog(splines, reason); } ... } </code>public class MoogDiver { Gradient gradient; List<Spline> splines; public void dive(String reason) { Gradient gradient = saturateGradient(); List<Spline> splines = reticulateSplines(gradient); diveForMoog(splines, reason); } ... }This exposes the temporal coupling by creating a bucket brigade. Each
function produces a result that the next function needs, so there is
no reasonable way to call them out of order. You might complain that
this increases the complexity of the functions, and you’d be right.
But that extra syntactic complexity exposes the true temporal
complexity of the situation. Note that I left the instance variables
in place. I presume that they are needed by private methods in the
class. Even so, I want the arguments in place to make the temporal
coupling explicit.
Here Mr. Martin is advocating that you take a more functional approach within your object. Seems weird but it works. It forces you to do things in order.
However, you’ll still have trouble with this class for a completely different reason. You are making both ChosenEstimates and YearPlan part of the object’s state. But YearPlan is, effectively, a function of ChosenEstimates. So YearPlan should literally be a function of ChosenEstimates. What does that mean?
It means don’t store YearPlan where you store ChosenEstimates. The two might disagree with each other (especially since you provide public setters). Force them to be consistent by making YearPlans getter return ReadExistingPlan(ChosenEstimates).
Done this way your Planner can’t be put into a weird state.
You could take this a step further and eliminate the extra work being done in your constructor. I prefer it when the constructor does no more than validate its arguments and set state. If I see more than that going on in the constructor I look for ways to take it out. A constructors job is to mutate the behavior of the objects methods. It’s not to do their work for them.
estimates could be your only state field and everything else in here would use that as their starting point. But ChosenEstimates could be as well. You’d just force whatever constructs Planner and estimates to also construct ChosenEstimates by passing estimates to a new EstimateCollection. Since nothing here is using estimates directly there isn’t a reason for it to be here naked.
It might seem like I’m devolving into a full blown code review (which we do on codereview.stackexchange.com not here) but by doing this time itself will be pushed out of the class. There is no getting things out of order now because doing things out of order won’t compile.
1
This answer extends on candied_orange’s answer. (Permission is given to combine the answers and delete this stub.)
The ReadExistingPlan method does not use the entire ChosenEstimates field. Rather, it only uses its StartDate and EndDate properties. Code clarity would improve if the caller passes in the right amount of information into ReadExistingPlan.
Once this is done, you will see that ReadExistingPlan method can possibly be moved to the PlannedYear class, since the “dependency” on ChosenEstimates field has been shrunken, from a full-blown object down to two DateTime fields, which could have been passed in as parameters.
ReadExistingPlan could be made a static method (factory method) on PlannedYear, or it could be made the constructor method. However, if this method needs additional from Planner, more thought would be needed.
4