1

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.

  • 1
    I would probably consider template method pattern to enforce desired ordering – gnat Oct 18 '19 at 16:59
  • 4
    I don't understand. What is this "ordering problem" you are having, and why is it a problem? – Robert Harvey Oct 18 '19 at 17:17
  • This is not really an issue; you are setting a private field and then calling an instance method that uses it - and this is all encapsulated within the class; it is pretty much expected that if you have private fields, they will have to be set before you can use them (this is what constructors are for, lazy initialization aside). Temporal coupling is a concern when you have several methods that must be called in a specific order. Also, generally speaking, you should avoid accessing the database in the constructor; they should do minimal work. – Filip Milovanović Oct 19 '19 at 14:36
  • @RobertHarvey The "ordering problem" is in the constructor of Planner(). ChosenEstimates is created, then YearPlan is created on the next line. To create YearPlan, ChosenEstimates must be set. However, this isn't obvious because in the constructor it's just two seemingly separate method calls.If another developer were to switch the order, so that YearPlan was created first, the code would compile but it would crash. – MSOACC Oct 21 '19 at 10:30
3

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

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:

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.

  • Thanks for the reply and for the other suggestions to improve my code. There's a few changes I can make from this on top of fixing the ordering problem. – MSOACC Oct 21 '19 at 10:50
2

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.

  • Well you're right that Planner doesn't need to have access to the public EstimateCollection.Estimates. This could be done thru a Duration interface. But that (and a host of other things I'd mention in a code review - like real encapsulation) doesn't really speak to the issue of: "Order Matters". That's more of a Interface Segregation Principle issue. – candied_orange Oct 19 '19 at 12:01
  • @rwong You're right that I could pass only the Start and EndDate to ReadExistingPlan(). What would be best if, for example, I needed 3+ parameters for ReadExistingPlan()? In that case would I be better supplying the entire ChosenEstimates field? – MSOACC Oct 21 '19 at 10:46
  • @candied_orange I am curious what other changes you'd applying to this in a full code review. Should I post this to the code review stackexchange site? – MSOACC Oct 21 '19 at 10:47
  • First be sure you have a good on topic question before you post there. I try to help the sites stay focused by avoiding treating them all the same. – candied_orange Oct 21 '19 at 11:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.