Promote Dependencies To Parameters

Photo by Emile Perron on Unsplash

Yesterday we discovered why we ought to favour unit tests over encapsulation. 

For our example, we had a CalculateStreak() method from an eLearning platform. Again, here is the listing:

  public int CalculateStreak(int streak, IEnumerable<StudentQuiz> quizzes)
  {
     var thisWeekFirstDay = _dtSvc.GetStartOfWeek(_dtSvc.UtcNow);
     var lastWeekQuizzeDates = quizzes.Where(q => q.startedAt >= thisWeekFirstDay.AddDays(-7) && q.startedAt < thisWeekFirstDay)
                                      .GroupBy(q => q.startedAt.Date);
     var thisWeekQuizzeDates = quizzes.Where(q => q.startedAt >= thisWeekFirstDay)
                                      .GroupBy(q => q.startedAt.Date);

     var lastWeekContinuity = lastWeekQuizzeDates.Count();
     var thisWeekContinuity = thisWeekQuizzeDates.Count();

     if (lastWeekContinuity < STREAK_CONTINUITY) streak = 0;
     if (thisWeekContinuity >= STREAK_CONTINUITY) streak++;

     return streak;
  }

This method comprises regular legacy code, nothing special. We get the gist of what it does, i.e. calculate the length of a learning ‘streak’ based on quizzes, but it is not that clear, at a glance, how it does its work. Yesterday we wanted to characterise the behaviour of CalculateStreak() by introducing unit tests. However, the method was private, and we could not call it in a test. We have fixed this problem by declaring it public, at the cost of compromising encapsulation a little.

Now we’re hitting the next obstacle. We should not be surprised; the standard operating procedure when trying to write unit tests for legacy code is to move from one dependency that we must break to another. That’s fine; ultimately, we’ll get there.

So which line poses the next stumbling block?

It’s 

  var thisWeekFirstDay = _dtSvc.GetStartOfWeek(_dtSvc.UtcNow);

_dtSvc is a reference to a class member of type DateTimeService. DateTimeService implements interface IDateTimeService. Good news should we need to mock or stub the service.

It appears the method invokes _dtSvc.GetStartOfWeek() to calculate the ‘first day of this week’ – the variable name could read more naturally, maybe firstDayOfTheWeek? 

_____________________

Aside: There is another problem with this line of code. It smacks of Feature Envy. The call to GetStartOfWeek() takes as argument a term that is also part of _dtSvc, specifically _dtSvc.UtcNow.

A better way to express this behaviour would have been to add a parameterless GetStartOfWeek() overload that internally makes a forwarding call to the parameterised overload:

  public DateTimeOffset GetStartOfWeek()
  {
     return GetStartOfWeek(UtcNow);
  }

The call in CalculateStreak() then reduces to

  var thisWeekFirstDay = _dtSvc.GetStartOfWeek();

We could go even further and condense this into a getter property.

Isn’t that much nicer?! 

_____________________

Let’s return our attention back to the call to _dtSvc.GetStartOfWeek().

Stubbing the DateTimeService for a single value seems a bit of overkill. Is there an easier way?

Yes, there is. 

We could make thisWeekFirstDay a parameter on CalculateStreak() and move the call to the DateTimeService outside the method. It would mean that for unit testing, we don’t have to stub DateTimeService; we could pass thisWeekFirstDay argument values, as appropriate. 

  public int CalculateStreak(int streak, IEnumerable<StudentQuiz> quizzes, DateTimeOffset thisWeekFirstDay)
  {
     var lastWeekQuizzeDates = quizzes.Where(q => q.startedAt >= thisWeekFirstDay.AddDays(-7) && q.startedAt < thisWeekFirstDay)
                                      .GroupBy(q => q.startedAt.Date);
     var thisWeekQuizzeDates = quizzes.Where(q => q.startedAt >= thisWeekFirstDay)
                                      .GroupBy(q => q.startedAt.Date);

     var lastWeekContinuity = lastWeekQuizzeDates.Count();
     var thisWeekContinuity = thisWeekQuizzeDates.Count();

     if (lastWeekContinuity < STREAK_CONTINUITY) streak = 0;
     if (thisWeekContinuity >= STREAK_CONTINUITY) streak++;

     return streak;
  }

We can turn the existing version of CalculateStreak() into a forwarding overload to ensure those existing callers of CalculateStreak() work as before:

  public int CalculateStreak(int streak, IEnumerable<StudentQuiz> quizzes)
  {
     var thisWeekFirstDay = _dtSvc.GetStartOfWeek(_dtSvc.UtcNow);
     return CalculateStreak(streak, quizzes, thisWeekFirstDay);
  }

Promoting a difficult-to-stub local variable to a parameter has significantly simplified the process of arranging unit tests for our method. Additionally, the data retrieval logic of the DateTimeService didn’t gel well with the rest of the behaviour in CalculateStreak() which is business logic solely concerned with how we calculate the length of a student’s streak.