Boolean method parameters are evil. This is the eighth in a series of posts on Code Gardening - the practice of making small improvements as you see them while you are working.

Boolean values are extremely useful - we use them all the time.

Yet I’ve become firmly convinced that they’re overused and that in some places they’re a very bad idea.

Consider this method, from when we discussed code documentation:

public Course FindCourseByCode(string code, bool throwIfMissing)
{
    code.MustNotBeEmpty(nameof(code));
    var result = _courses.FirstOrDefault(c => c.HasCode(code));
    if (result == null && throwIfMissing)
    {
        throw new InvalidOperationException("Course not found");
    }

    return result;
}

This method is easy enough to read, but think about the case when the method is consumed:

var course = FindCourseByCode("COSC201", true);

Without referring to the actual implementation, the meaning of true is completely opaque. You either magically know what it means, or you have to interrupt your current task to go and find out.

I’ve found this issue crops up almost without fail when methods have bool parameters. There are exceptions, of course, but a bool argument is a strong code smell that should be addressed in the majority of cases.

When there is a single bool parameter, the most likely solution is to create two variations of the method with names that reflect their differing behavior.

First, here’s a variation for when throwIsMissing is false - note both the new name of the method to indicate that it might not return a course and the presence of code documentation:

/// <summary>
/// Find a course based on its unique identifying code
/// </summary> 
/// <param name="code">
/// The (case insensitive) unique code to use for the search.
/// </param>
/// <returns>A matching course, if found; null otherwise.</returns>
public Course FindOptionalCourseByCode(string code)
{
    Require.MustNotBeBlank(code, nameof(code));

    var result = _courses.FirstOrDefault(c => c.HasCode(code));
    return result;
}

Secondly, here’s a variation for when throwIfMissing is true, again with an indicative name:

/// <summary>
/// Find a course based on its unique identifying code
/// </summary>
/// <param name="code">
/// The (case insensitive) unique code to use for the search.
/// </param>
/// <returns>
/// The course that was found.
/// </returns>
/// <exception cref="InvalidOperationException">
/// If no course with this code is found.
/// </exception>
public Course FindMandatoryCourseByCode(string code)
{
    Require.MustNotBeBlank(code, nameof(code));

    var result = FindOptionalCourseByCode(code);
    if (result != null)
    {
        return result;
    }

    throw new InvalidOperationException("Course not found");
}

This second method implementation is in terms of the first, guaranteeing consistency of behavior both now and in the future. A completely independent implementation would run the risk of the methods diverging in the future as the system evolved.

With those two methods in place, consumers of the methods are explicit about the required behaviour:

var course = FindMandatoryCourseByCode("COSC201");

As I’ve discussed this idea with various developers, I’ve found general agreement on the principle but a lot of disagreement on method naming.

Here, I’ve used FindMandatoryCourseByCode and FindOptionalCourseByCode, but some dislike those names because they might be ambiguous, as though “Mandatory Course” and “Optional Course” are different kinds of course.

A suggested alternative would be more explicit, using names like FindCourseOrNullByCode and FindCourseOrErrorByCode.

One team that I worked on had a global convention that Get methods were for when the thing must already exist, and Find methods were for when the thing might not exist - they would have used FindCourseByCode and GetCourseByCode.

Another team had a similar convention, but based on the terms Search and Load (attempting to avoid the issue of developers treating find and get as synonyms). They would have used SearchForCourseByCode and LoadCourseByCode.

Regardless of the naming standard you choose, removing a bool parameter by replacing the method with two related variations will improve your code and make it easier to read and maintain.

Comments

blog comments powered by Disqus