-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\$
  • 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\$ – 200_success Nov 11 '16 at 18:33
  • \$\begingroup\$ @200_success My bad. Is the new title better? \$\endgroup\$ – Nathan GoFundMonica Arthur Nov 14 '16 at 14:16
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\$
  • 1
    \$\begingroup\$ I wouldn't consider this controversial at all, but rather good coding practice :) \$\endgroup\$ – Mike Brant Nov 11 '16 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\$ – Nathan GoFundMonica Arthur Nov 14 '16 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, privacy policy and cookie policy

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