Forem: Nick Rogers

Comment Why, Not What

Nick Rogers — Fri, 10 Jul 2020 11:42:07 +0000

I've been involved with interviews and the hiring process at DealerOn for a number of years. Part of this process is to administer a code test, where we give the candidate their choice of a few non-strenuous coding challenges, and they take a crack at solving the prompt (at their leisure) and sending us their code that they wrote. There are some patterns that we commonly notice on junior candidates fresh out of college, most notably in the comments in their code.

Students often have it hammered into them by their computer science professors to leave lots of comments in their code, sometimes to the point of a student being penalized if the grader feels there aren't enough of them in his or her submitted work. The problem is, a high quantity of low-quality comments can often make the code more confusing than if there were none at all.

Take a look at this example:

/// <summary>
/// This method gets the sales targets for 
/// each day of the week from the database.
/// </summary>
/// <param name="employeeName">The employee's name</param>
/// <returns>A Dictionary</returns>
public Dictionary<string, decimal> GetSalesTargets(string employeeName) 
{
  // create a new array
  var employees = new Dictionary<string, decimal>(); 
  if (employeeName != "Charlie") // check if employee is Charlie
  {
    // employee is not Charlie
    // add Monday to the array
    employees.Add("Monday", GetSalesTarget("Monday", employeeName)); 
    // add Tuesday to the array
    employees.Add("Tuesday", GetSalesTarget("Tuesday", employeeName)); 
    // add Wednesday to the array
    employees.Add("Wednesday", GetSalesTarget("Wednesday", employeeName)); 
    // add Thursday to the array
    employees.Add("Thursday", GetSalesTarget("Thursday", employeeName)); 
    // add Friday to the array
    employees.Add("Friday", GetSalesTarget("Friday", employeeName)); 
  }
  else
  {
    // employee is Charlie
    // add Tuesday to the array
    employees.Add("Tuesday", GetSalesTarget("Tuesday", employeeName)); 
    // add Wednesday to the array
    employees.Add("Wednesday", GetSalesTarget("Wednesday", employeeName)); 
    // add Thursday to the array
    employees.Add("Thursday", GetSalesTarget("Thursday", employeeName)); 
    // add Friday to the array
    employees.Add("Friday", GetSalesTarget("Friday", employeeName)); 
    // add Friday to the array
    employees.Add("Saturday", GetSalesTarget("Saturday", employeeName)); 
  }
  // return the array
  return employees;
}

Did you spot all the problems with those comments? For starters, the variable we're adding the days of the week to is a Dictionary, not an array, and why is it called employees if it's working with the days of the week? Also, comments like create a new array, add to the array, and return the array are unnecessary because any programmer can look at the syntax of those lines and clearly understand what is happening, even if the language itself is unfamiliar. The main problem with this code is, these comments are answering "What?" when they should be answering "Why?".

But why?

You probably found yourself saying "but why?" at several points reading the above code. The code has comments, but all they are doing is explaining what the code is doing at that moment; they do a poor job of explaining the things that actually need clarification. Imagine a new developer tasked with making a slight modification to this code. They may be struck with questions such as:

For what purpose are we retrieving this data?
Shouldn't Saturday and Sunday always be included in the returned value?
Why is Charlie a special case?
What format must the employee's name be in for the parameter? Will it work if the employee's last name is given as well?
What service will be using this data?

None of these questions are answerable with the code as it currently is, leaving our poor developer to scratch his or her head for a while and then ask for help. Let's rewrite this method to be cleaner and to answer the above questions.

/// <summary>
/// This method gets the sales targets for a given employee for 
/// each day of this employee's work week, formatted for displaying 
/// on the company's sales dashboard.
/// </summary>
/// <param name="employeeFirstName">The employee's first name, as recorded 
/// in this company's LDAP server.</param>
/// <returns>A Dictionary mapping the employee's work days to their 
/// sales goal for that individual day of the week.</returns>
public Dictionary<string, decimal> GetDailySalesTargetsForEmployee(string employeeFirstName) 
{
  var salesTargets = new Dictionary<string, decimal>();
  var normalSchedule = new[] { "Monday", "Tuesday", "Wednesday", "Thursday", "Friday" };
  var modifiedSchedule = new[] { "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday" };

  if (!EmployeeWorksModifiedSchedule(employeeFirstName))
  {
    foreach (var day in normalSchedule) 
    {
      salesTargets.Add(day, GetSalesTarget(day, employeeFirstName));
    }
  }
  else
  {
    foreach (var day in modifiedSchedule) 
    {
      salesTargets.Add(day, GetSalesTarget(day, employeeFirstName));
    }
  }
  return salesTargets;
}

This can, of course, be condensed further by using a ternary expression or some LINQ. Regardless, this version of the method is much more readable, and a new developer would have a better idea of what's going on, saving everyone some time. It's amazing how much more clear the code can be with a modest refactor and by using more specific naming. This code answers most questions an otherwise unfamiliar developer would have, all without a single comment in the method body.

Comments can become out of date due to inattentive developers. Did you notice in the first example, in addition to the array/Dictionary mistakes, there's a copy/paste error, probably by someone that copied the original method in order to support Saturdays, and forgot the change the final comment. This is a tame example, but there is a phenomenon that, when doing any rote task such as copying and pasting a line of code over and over, more errors are made on the final iteration than any other. This is the so-called "Last-Line Effect". Comments become invisible when you're in the code for too long, but it happens to the best of us. At DealerOn we have a pretty infamous method still in our codebase which has comments stating that the method checks a string for the word "checked" and returns a Boolean, when it actually checks a string for a different word entirely and returns 0 or 1. This code isn't referenced anymore, but it still exists as a cautionary tale.

Comment Where Necessary

To be clear, I'm not saying there is no place for comments in modern development, nor do we at DealerOn judge code test submissions harshly for using too many comments. We suggest candidates do the code prompt like they would a school assignment, and if they are accustomed to verbose comments, that's perfectly fine. Speaking for myself however, if I'm unsure whether or not to leave a comment, I ask myself if what the code is doing is unclear, and if it can't be made more clear with a refactor. That last part is important: if the code itself can be made easier to understand, do it!

If a refactor isn't in scope for the unit of work, I use comments when something looks counter-intuitive, like when a method needs to account for an edge-case but it's unclear why. They're mostly "Trust me, I know what I'm doing" comments. There's a lot to be said about self-documenting code, code which is clear and easy to read, without resorting to method body comments.

Resolving Dependencies at Runtime in ASP.NET Core

Nick Rogers — Fri, 01 Nov 2019 17:32:16 +0000

ASP.NET Core supports dependency injection (DI), a great pattern for decoupling classes which would normally perform multiple functions inside the same region of the application. The dependency injection engine of ASP.NET Core, known as a service container, is a functional, albeit basic, way to create a well-factored application with separated concerns. With little additional code, developers can easily select dependencies to add to the container when the conditions are known at application startup, such as whether the app is running from a Pre-Prod or a Production environment.

But what if we want to use this cool thing on a condition that we'd only know at runtime, after the Startup code is long gone? For example, what if we wanted to use different implementations of an interface based on a user's type of account or the user's location? There are additional third-party service containers that augment Microsoft's stock service container with advanced functionality, and in fact, Microsoft's own docs suggest a number of solutions if theirs doesn't suit your needs. But come on, do you really want to have two of these in your app? There's an easy way to resolve dependencies at runtime with our old friend, the factory pattern.

Let's say we are writing a simple app to determine the weather for a location, determined by a postal code the user enters into a text box. One of the requirements is to support both United States ZIP codes and Canadian postal codes, which are formatted differently. Let's also say that, unfortunately, each location requires the use of a different API between the US and Canada, so we can't pick a single implementation at startup and register it. Not to fear, there is an overload on the .Add() methods for the service container that accepts a Func<IServiceProvider, TService> parameter. This means that instead of specifying a concrete implementation for the TService type, we can instead inject a runtime function that takes any number of parameters and returns an object of type TService! Here's our first crack at taking this approach:

First up, we have our central service. The most important thing to take note of here is the IWeatherService being injected into the class constructor on line 22.

Next we have the WeatherService implementation, just wrapping out calls to WeatherServiceApi and WeatherServiceParser factory classes.

And finally, we have our central player, the Program.cs where the IWeatherServiceApi and IWeatherServiceParser factories are configured with a singleton registration function on app start up.

When we run this app, a function taking a string as a parameter to determine which API to use is injected into the IWeatherService implementation, and it runs the appropriate API based on the postal code that the user entered. This works fine, but there's a bit of a gotcha here. Notice in our injected function we are returning a new implementation each time the factory function is called. This is fine if it's only called a small number of times, but if it runs in a long loop or if the returned service has dependencies of its own that need to be injected, we could be in trouble.

So what can we do? Well, did you notice that we also have access to the service container through the service variable inside the function in the example above? What we can do is query the container for all services implementing a certain type and then pick the one appropriate for the runtime condition using some LINQ.

Using the service container in this way allows us to take full advantage of the power of dependency injection. Instead of returning a new object, the container returns a reference to the concrete implementation for the requested type, as defined at application startup. Clear and well thought-out strategies for dependency injection like this one can help improve reliability and testability for many apps, small and large.

"Factory" by Related Fluid Power is licensed under CC BY 2.0