How to Refactor a Method With Optional Params

While refactoring and optimising legacy code I've ran into a method signature (optional parameters) inconsistency which caused performance issues as part of the expensive operation was being done without developer intention.

class PageRepository
{
    ...
    public function find($pageId, $prerender = false)
    {
        ...
        if ($prerender === true) {
            $page->prerender();
        }
        ...
    return $page;
    }

    public function findOrFail($pageId, $prerender = true)
    {
        $page = $this->find($pageId, $prerender);

        if ($page === null) {
            throw new Exception("Page $pageId not found.");
        }

    return $page
    }
    ...
}

Our PageRepository::find method had an optional parameter called $prerender which was set to false by default. The problem is that the method PageRepository::findOrFail has the same optional parameter but its default value set to true!

If you're not careful reading method signature and default param values you can easily make a mistake and "prerender" a page (which is an expensive operation) without wanting or needing it.

Methods with optional "flag" parameters can potentially be dangerous and can be hard to glance at.

There are a couple of things that can be done here in order for it to be better and more readable.

1. Change optional parameter default values to the same value

This one is a no brainer. Just change the PageRepository::findOrFail optional $prerender default value to false (or vice-versa).

2. Specify optional param value when calling methods that have them

Just specify a value explicitly so anyone reading your code (even you a couple of months in the future) will know that you've set it and used it intentionally.

$pageRepository = new PageRepository();

$page = $pageRepository->find($pageId, false);

Or even better, use the variable with a semantic meaning when calling the method.

$pageRepository = new PageRepository();

$page = $pageRepository->find($pageId, $prerender = false);

I know that some linters or IDEs may highlight the $prerender param but there are several ways to deal with it and I'm leaving it at your discretion to chose a way how to handle it.

3. Refactor the method with optional "flag" parameter into two distinct methods

Remove the $page->prerender() call from the PageRepository::find method and extract it to a separate PageRepository::findAndPrerender method.

class PageRepository
{
    ...
    public function find($pageId, $prerender = false)
    {
        ...
        if ($prerender === true) {
            $page->prerender();
        }
        ...

        return $page;
    }

    public function findAndPrerender($pageId)
    {
        $page = $this->find($pageId, $prerender);

        if ($page === null) {
            throw new Exception("Page $pageId not found.");
        }

        return $page;
    }
    ...
}

This way you have to make an intentional effort to chose which functionality (or rather which side-effect) you require.

This was just a quick refactoring tip which I’m usually enforcing suggesting while doing code reviews in order to make code easily understandable and to avoid confusion.