Comment by JD Lien on "5 Tips to Write Clean Code in JavaScript"

Great article, Simon!

I am finding it hard to universally agree with the notion of not making functions with boolean (flag) arguments. If you had to make a different function for every variation of a method, wouldn’t you often end up with multiple functions with a lot of duplicate or similar code in them?

I suppose the obvious solution to this is to break such functions up into smaller methods, but then in some cases wouldn’t you end up with several methods where one would do that just accepts a few flags?

Would you consider it acceptable to have a parent function with flags and some simple methods to call the parent just for readability’s sake?

If you can give further clarifications or examples, that would be great!

Hi JD! And thanks!

I'd argue that boolean parameters should never be used. Of course, everything is contextual :)

It all boils down to having the code readable and easy to reason about. Would you not prefer three small functions that do one thing and one thing only, over one function with two boolean parameters?

It might be tempting to write that bigger function. But then later someone else might want to add a flag to the function and then you have three flags. Refactoring this will be a pain. And when you read the code you need to keep in mind what all these booleans represent.

For languages with named arguments (such as Python and Scala) it makes more sense to use flags, assuming the linter forces you to always name boolean arguments.

For some occasions, it might make sense to do something like this: gist.github.com/simeg/250fc0e300b75e4bd9fbc6161e0…

Notice the difference in visibility. Although I would advise against this if you're writing new code. This is a code smell. For this case, polymorphism should be used.

Thanks for asking this! :) I hope I make sense. If you have specific examples to discuss please share!

Simon Egersand 🎈

Thank you for the thoughtful response! I don’t have a specific example in mind now, but I guess the kind of scenario where this may make sense to me (to use a boolean argument) is the sort of thing where you have a lot of setup to do, but then you might want to go down a couple of roads with what it returned.

I guess the “setup” work could be extracted to another function. I know the Uncle Bob Clean Code approach favours many small, single purpose functions over few large, monolithic ones.

I’ve been writing lots of functions with all kinds of boolean arguments for a long time (especially in ColdFusion custom tags), and I only recently heard of this concept that such arguments are a code smell. I guess it’s time for me to have a second look at how I write these sorts of things!

Simon Egersand 🎈

I just found this real function today. It grabs a user's photo from an API, but if you wanted to force the photo to update when it normally wouldn't, I created a boolean option for that.

public function saveUserPhoto(User $user, bool $forceUpdate = false): void
{
    if ($this->userHasUpdatedPhoto($user)) return;

    if (!$this->userHasPhoto($user) || $forceUpdate) {
        try {
            $response = $this->graphServiceClient->usersById($user->id)->photo()->content()->get();

            $photoContent = $response->wait();

            $this->savePhoto($user->id, $photoContent);
        } catch (ApiException $ex) {
            echo $ex->getMessage();
        }
    }
}

I just refactored the code into this - the drawback is that I had to check existing implementations to ensure they were calling the correct function, but in the end the code should be cleaner to read and the complexity is probably about the same in exchange for a couple extra lines of code in the file.

    /** Process and save a single user's photo given a userId. */
    public function saveUserPhoto(User $user): void
    {
        if ($this->userHasUpdatedPhoto($user)) return;

        try {
            // Get the photo from the Graph API
            $response = $this->graphServiceClient->usersById($user->id)->photo()->content()->get();

            $photoContent = $response->wait();

            $this->savePhoto($user->id, $photoContent);
        } catch (ApiException $ex) {
            echo $ex->getMessage();
        }
    }

    /** Process and save a single user's photo unless they already have a photo. */
    public function saveUserPhotoIfNoPhoto(User $user): void
    {
        if ($this->userHasPhoto($user)) return;

        $this->saveUserPhoto($user);
    }

Search Hashnode