0

When browsing the source code of a project I am contributing to, I find this line of code:

$id or $id = $this->id;

It seems to be working normally, and it is kinda like a workaround for the lack of null coalesce operator/handler in PHP 5. the 'or' keyword is able to execute such code. However, I wonder if this is a good programming practice in PHP. What do you think? Is it a good idea to write such ($x or $x = $y) code?

TylerH
  • 20,799
  • 66
  • 75
  • 101
Lord Yggdrasill
  • 3,197
  • 4
  • 26
  • 42
  • 5
    Personally, I don't think it's very readable. I'm also not sure how it handles cases where $id is falsey but not null. Now PHP 7 is here though, hopefully these hacks become less common. – Chris Apr 13 '16 at 18:41
  • This question seems to be more geared towards the type of content present over at: http://programmers.stackexchange.com/ – Tyler Benzing Apr 13 '16 at 18:44
  • 2
    This tells me, NO `Notice: Undefined variable: id` – AbraCadaver Apr 13 '16 at 18:44
  • I dont think it to be a baaaaaaad practice. I mean, javascript uses that a lot (ok, not the best of comparisons here lol) but C# now has a null operator that can be used like this `var data = Obj?.Property?.Method()`. That `or` statement would probably be rewritten as `$id = isset($id) ? $id : $this->id`, so I see it as a short version for it... – Vitor Rigoni Apr 13 '16 at 18:45
  • @AbraCadaver: There actually isnt a notice error, since this code comes from a class method that accepts $id as a parameter with default value null: public function xml($id = null). – Lord Yggdrasill Apr 13 '16 at 18:48
  • Depends then, if `0` is a valid id and passed, this will fail and set `$id = $this->id` – AbraCadaver Apr 13 '16 at 18:51
  • Good point. I think the intent of that code is to check for null value, but of course in PHP many other values evaluate to false, such as 0, empty string and empty array. – Lord Yggdrasill Apr 13 '16 at 18:54

1 Answers1

2

This question is mainly opinionated, but you managed to raise a valid question.

Just for quick reference, in PHP 7 there is the null coalesce operator that does this quickly and effective in 1 short code:

$id = $id ?? -1; or chained $id = $id ?? $this->id ?? -1

As this compares every ?? {arg} ?? {arg} with isset()


But why don't do it with an or statement?

Pros

  1. It's short (1-lining)
  2. Fairly fast and easy to read.
  3. Less complex than ternary operators
  4. Most likely faster in code-parsing

Cons

  1. I'ts very deceiving

If you do not fully understand how type comparisons work in PHP, it will eventually bite you. If you thought of this like isset() you are dead wrong and would read the code incorrectly because of it it. For this reason I would vote against using this method.


expression       if       isset    empty

$x = "";         FALSE    TRUE     TRUE
$x = null;       FALSE    FALSE    TRUE
var $x;          FALSE    FALSE    TRUE
$x (undefined)   FALSE    FALSE    TRUE
$x = [];         FALSE    TRUE     TRUE
$x = ['a', 'b']; TRUE     TRUE     FALSE
$x = false;      FALSE    TRUE     TRUE
$x = true;       TRUE     TRUE     FALSE
$x = 1;          TRUE     TRUE     FALSE
$x = 42;         TRUE     TRUE     FALSE
$x = 0;          FALSE    TRUE     TRUE
$x = -1;         TRUE     TRUE     FALSE
$x = "1";        TRUE     TRUE     FALSE
$x = "0";        FALSE    TRUE     TRUE
$x = "-1";       TRUE     TRUE     FALSE
$x = "php";      TRUE     TRUE     FALSE
$x = "true";     TRUE     TRUE     FALSE
$x = "false";    TRUE     TRUE     FALSE

For a full list of the table you can see this link.


It works like !empty(), see the following example:

if(($id or $id = $this->id) > 0){
  echo "Greater then 0";
}

if(($id = !empty($id) ? $id : $this->id) > 0){
  echo "Greater then 0";
}

Most people are not aware of this. The code isn't obvious to read as well. So while I am perfectly able to read the code now, It will be a bit more confusing when I read over it next month.

There is also a question in this code if $this->id is actually set, could create bugs in code.

Community
  • 1
  • 1
Xorifelse
  • 7,878
  • 1
  • 27
  • 38