Jeff Swensen Hot takes and mistakes

Be Explicit

Too often code is written with little empathy towards those who will follow us and need to understand, modify and replace it. After the functional priorities (correct, stable, testable, etc.) your highest priority should be writing readable code that is easy to understand. Let’s look at a simple example.

public HttpResponseMessage GetCourses(int classroomId, int professorId = 0) {

    var classroom = ClassroomRepo.Find(classroomId);
    if (classroom == null) {
        throw new HttpResponseException(HttpStatusCode.NotFound);
    }

    var professor = ProfessorRepo.Find(professorId);

    var courses = classroom.Courses(professor);

    return Request.CreateResponse(courses);
}

While this code probably works fine, it relies on a few bad assumptions and obfuscates functionality behind those assumptions. First, the default value of 0 for the professorId parameter implies a professor isn’t required for this method to work properly. In other words, our database unique IDs probably start at 1 and go up. Second, the code assumes the ProfessorRepo.Find() method will return some usable value (that Classroom.Courses() knows how to handle) when the given professorId isn’t found in the database (rather than throwing an Exception).

Of course, I’m not arguing you shouldn’t write code that’s aware of how dependent code work, that would be unreasonable. My advice is to make it easier for future readers of your code to understand that underlying behavior without first having to dig through it. With that in mind, let’s rewrite the above snippet and get rid of some of those hidden assumptions.

public HttpResponseMessage GetCourses(int classroomId, int? professorId = null) {

    var classroom = ClassroomRepo.Find(classroomId);
    if (classroom == null) {
        throw new HttpResponseException(HttpStatusCode.NotFound);
    }

    Professor professor = null;
    if (professorId.HasValue) {
        professor = ProfessorRepo.Find(professorId.Value);
        if (professor == null) {
            throw new HttpResponseException(HttpStatusCode.NotFound);
        }
    }

    var courses = classroom.Courses(professor);

    return Request.CreateResponse(courses);
}

Not much has changed, right? We changed the type of the professorId parameter to a nullable integer and we changed its default value to null. We also explicitly checked for that default null value before attempting to load a Professor instance from the Repository.

An explicit indication (default of null) and check (== null) of whether an optional parameter has a value makes the behavior of the underlying code that much more obvious to an unfamiliar reader. I no longer have to know that 0 is not a valid value for a professorId. I also no longer have to know what ProfessorRepo.Find() is going to do with that invalid value. We’ve also managed to avoid an unnecessary round trip to the database to confirm that a professorId of 0 is invalid (assuming Find() doesn’t do a short-circuit check).