-1
\$\begingroup\$

I have a method that's called thousands of times during one process. Before optimization, it called the database on every execution. After optimization, it only calls the database when required. However, this optimization came at the expense of readability.

Method before optimization:

private function addHandleToOrg( $org )
{
    $committedHandle = $this->orgRepository->getCommittedHandle( $org->org_code );
    $dbHandle = $this->orgRepository->getHandleByOrgCode( $org->org_code );
    $dataHandle = $this->extractDataHandle( $org );

    if ( $committedHandle ) {
        $org->handle = $committedHandle;
    } elseif ( $dbHandle ) {
        $org->handle = $dbHandle;
    } else {
        $org->handle = $dataHandle;
    }

    return $org;
}

Method after optimization:

private function addHandleToOrg( $org )
{
    $committedHandle = $this->orgRepository->getCommittedHandle( $org->org_code );

    if ( $committedHandle ) {
        $org->handle = $committedHandle;
    } else {
        $dbHandle = $this->orgRepository->getHandleByOrgCode( $org->org_code );

        if ( $dbHandle ) {
            $org->handle = $dbHandle;
        } else {
            $dataHandle = $this->extractDataHandle( $org );

            if ( $dataHandle ) {
                $org->handle = $dataHandle;
            }
        }
    }

    return $org;
}

How can I make this method more readable? I'm open to splitting it into multiple methods if needed, so long as the optimization gains are not sacrificed.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ "If-else optimization" is a very common request and therefore not appropriate as a title. Please retitle the question to state the task performed by the code, as per site guidelines. See How to Ask. \$\endgroup\$ Nov 11, 2016 at 18:33
  • \$\begingroup\$ @200_success My bad. Is the new title better? \$\endgroup\$ Nov 14, 2016 at 14:16

2 Answers 2

4
\$\begingroup\$

This is controversial for many people, but I'm a fan of early-exit. PHP allows you to have more than one return statement, so you can improve readability (and reduce cyclomatic complexity) by using that principle:

  private function addHandleToOrg( $org )
  {
      $committedHandle = $this->orgRepository->getCommittedHandle( $org->org_code );

      if ( $committedHandle ) {
          $org->handle = $committedHandle;
          return $org;
      }

      $dbHandle = $this->orgRepository->getHandleByOrgCode( $org->org_code );

      if ( $dbHandle ) {
          $org->handle = $dbHandle;
          return $org;
      } 

      $dataHandle = $this->extractDataHandle( $org );

      if ( $dataHandle ) {
          $org->handle = $dataHandle;
      }

      return $org;
  }

Here, by using early-exit you've improved readability, reduced arrow code and made the return value a little clearer.

Optionally, instead of returning the parameter you are taking in, you could pass by reference:

  private function addHandleToOrg( &$org )
  {
      $committedHandle = $this->orgRepository->getCommittedHandle( $org->org_code );

      if ( $committedHandle ) {
          $org->handle = $committedHandle;
          return;
      }

      $dbHandle = $this->orgRepository->getHandleByOrgCode( $org->org_code );

      if ( $dbHandle ) {
          $org->handle = $dbHandle;
          return;
      } 

      $dataHandle = $this->extractDataHandle( $org );

      if ( $dataHandle ) {
          $org->handle = $dataHandle;
      }
  }

Which reduces your footprint a little bit but depending on how your upstream code handles it this is either a large breaking change or a minimal one. It would avoid code that looked like:

$org = $this->orgRepository->getOrg($orgId);
$org = addHandleToOrg($org);

(The second line being the point) It just looks strange.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I wouldn't consider this controversial at all, but rather good coding practice :) \$\endgroup\$
    – Mike Brant
    Nov 11, 2016 at 23:09
  • \$\begingroup\$ Ended up going with the early return approach. I think the logic in this method is simple enough in your refactoring that the early returns don't present a problem. Accepted. \$\endgroup\$ Nov 14, 2016 at 14:20
3
\$\begingroup\$

Your second version is better in that it can do less unnecessary work. I think it could be more compact, though. I don't advise splitting it into multiple methods, as that would just make the code more verbose and harder to follow.

One way to write it is using side-effect assignments within the conditionals:

private function addHandleToOrg( $org )
{
    if (($committedHandle = $this->orgRepository->getCommittedHandle($org->org_code))) {
        $org->handle = $committedHandle;
    } elseif (($dbHandle = $this->orgRepository->getHandleByOrgCode($org->org_code))) {
        $org->handle = $dbHandle;
    } elseif (($dataHandle = $this->extractDataHandle($org))) {
        $org->handle = $dataHandle;
    }
    return $org;
}

A better approach is to take advantage of the short-circuit behaviour of the || operator.

private function addHandleToOrg( $org )
{
    $handle = $this->orgRepository->getCommittedHandle($org->org_code)
           || $this->orgRepository->getHandleByOrgCode($org->org_code)
           || $this->extractDataHandle($org);
    if ($handle) {
        $org->handle = $handle;
    }
    return $org;
}

Depending on the intended behaviour (will the function ever be called with an $org that already has an $org->handle?), you might even simplify it down to this:

private function addHandleToOrg( $org )
{
    $org->handle = $this->orgRepository->getCommittedHandle($org->org_code)
                || $this->orgRepository->getHandleByOrgCode($org->org_code)
                || $this->extractDataHandle($org);
    return $org;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.