Clean Code: Avoiding “Order Matters” without passing member variables to private methods?
https://softwareengineering.stackexchange.com/questions/399855
-
03-03-2021 - |
Question
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.
Solution
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 303public 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:
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.
OTHER TIPS
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.