Skip to content

Instantly share code, notes, and snippets.

@aramonc
Last active August 29, 2015 14:01
Show Gist options
  • Save aramonc/ed06833a16249039dbf2 to your computer and use it in GitHub Desktop.
Save aramonc/ed06833a16249039dbf2 to your computer and use it in GitHub Desktop.
How bad is it to have code like this for readability & maintainability
<?php
if($paramFetcher['owner'] != null && ($owner = $userMgr->getById($paramFetcher['owner']))) {
$org->setOwner($owner);
}
?>
vs.
<?php
if($paramFetcher['owner'] != null) {
if($owner = $userMgr->getById($paramFetcher['owner'])){
$org->setOwner($owner);
}
}
?>
FINAL:
<?php
$owner = $this->getUserManager()->getById($paramFetcher['owner']);
if ($owner) {
$org->setOwner($owner);
}
?>
@ar4mirez
Copy link

Lol is so bad ! :(

@aramonc
Copy link
Author

aramonc commented May 20, 2014

lol which one? {1,3} or {7,11}?

EDIT: He said {1,3} is not as easy to read and harder to understand for someone new

@onel0p3z
Copy link

hmmm ... first thought was that the first way, imho, is best for both but then I started to doubt ... I'd go with #1 for only that, if you need to check more than that then go with #2

@JeffMatson
Copy link

I agree with the above.

@adamculp
Copy link

I would probably create an isOwner() function (containing two separate if blocks) and call it to do the checks, returning a bool.

Reusable, testable, self documenting, and less cyclomatic complexity.

@aramonc
Copy link
Author

aramonc commented May 21, 2014

Ended up changing it. I realized I was duplicating some code to get users and that if the parameter for getById() is null, it will return null. So there was no need to check $paramFetcher['owner'] individually.

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment