Discussion: avoid assignment in if() conditions?

artfulrobot commented 2 years ago

Reading the code, assignment in if statement conditions has caught me out a few times, things like if ($match = $savedMatches[x][y] ?? NULL)

Arguably,

I'm aware of it now and will look out for it, but I suppose a proposal might be:

That we say if ($var = 'assignment') is to be avoided.
this pattern is open to accidents as = can be confused with ==. If = is always wrong, then it's clearer.
this pattern makes code harder to scan (at least in my experience, it seems most projects I've dealt with generally avoid this pattern, so an if() is a question typically without side effects).
it costs an extra line of code to have the doing code outside of the if, but the project's general code style seems to lend itself to clarity over conciseness.

Notable exceptions - my personal view:

while ($line = fgets($fileHandle)) or while ($row = $dao->fetch()) are ok to me, because I think they're more common and avoiding the assignment in the while condition requires two additional, duplicated lines (one before the while, and one at the end of the loop, or worse before any continue).
if (preg_match('/pattern/', $subject, $matches)) { /* use $matches */ } is fine although it does assign to $matches.

I don't have very strong opinions on this, but you mentioned you were open to considerations so thought I'd document it.

highfalutin commented 2 years ago

Why don't we avoid assignment in if () conditions going forward, with the exceptions you mentioned. If you stumble across particularly egregious examples in existing code, submit a PR referencing this issue.

artfulrobot commented 2 years ago

Works for me.

highfalutin commented 2 years ago

How would you handle this?

Without assignment in the if condition: at least 8 lines, more if I add whitespace for readability:

$tag = $pair->getLocalObject();
if (empty($tag)) {
  continue;
}

$id = $tag->getId();
if (!empty($id)) {
  $ids[] = $id;
}

With assignment in the if condition: 3 lines:

if (($tag = $pair->getLocalObject()) && ($id = $tag->getId())) {
  $ids[] = $id;
}

I know I could save a line or two by making repeated calls to the same getters, but that seems like a waste of cpu. I'm OK with using the long version, but I wondered if you had thoughts about how to preserve some concision, @artfulrobot.

artfulrobot commented 2 years ago

Well I used to write Perl, which has to be the queen of all languages when it comes to demonstrating Kernighan's law, so why not go further:

($t = $p->getLocalObject()) && ($i = $t->getId()) && $ids[] = $i;

I don't think CPU cycles are at play here, I'm pretty sure these are compiled down to identical code or certainly a level of micro optimisation that is of no benefit to this project. To do either version 1,000,000 times takes typically less than 0.01s - How many tags are we dealing with here :-)

I'm not sure the purpose of the code sample though. Get tag, which might not exist. Then a tag that does exist might not have an ID? I think if you have 3 possible pathways (exists with ID, exists without ID, does not exist), the extra lines are worth it.

7 lines

There's also a 7 line version (if this is at the end of the loop, so the continue can be implied)

$tag = $pair->getLocalObject();
if ($tag) {
  $id = $tag->getId();
  if ($id) {
    $ids[] = $id;
  }
}

5 lines

There's also 5 SLOC:

$tag = $pair->getLocalObject();
if ($tag) {
  $ids[] = $tag->getId();
}
// outside loop
$ids = array_filter($ids);

4 lines

Or 4 lines (vs your original 3) and arguably just as readable as the verbose one if PSR12 is not important:

$tag = $pair->getLocalObject();
if (!$tag) continue;
$id = $tag->getId();
if ($id) $ids[] = $id;

I used to find $longWindedVerboseNames or worse, $long_winded_verbose_names annoying, but the readability pays off in the end, and the machine parse costs are negligible but the human parse benefits are much greater.

artfulrobot commented 2 years ago

Just stumbled across this https://processwire.com/api/ref/null-page/ which is an intersting approach to fluent codestyle, and allows you to do

$id = $pair->getLocalObject()->getId();
if ($id) {
  $ids[] = $id;
}

highfalutin commented 2 years ago

@artfulrobot your comments in https://github.com/lemniscus/osdi-client/issues/6#issuecomment-1246596977 are excellent. I'm new to Kernighan's law and 3v4l.org (whose name apparently is "leetspeak for eval", but looked to me like "3 vs 4 lines", appropriate to what we're discussing here!). You've convinced me that computer performance isn't the most relevant thing here. I still think that more lines can be harder to read than fewer lines, and I'm glad you mentioned PSR12 ― just so I can shake my fist at it. I am sad every time I have to make return; into

{
    return;
}

😣

but I feel it's best to follow Civi's code style.

I agree with you re: long names.

highfalutin commented 2 years ago

Oops, didn't mean to reopen. Anyway, interesting to read about the null object thing, thanks for sharing. I've used that kind of thing here and there in this extensions code. Might be good to keep this at the back of our minds and think about whether it's a useful pattern to implement consistently.

lemniscus / osdi-client