New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Stop using empty() as much as possible? #8397

Closed
chinpei215 opened this Issue Mar 3, 2016 · 21 comments

Comments

Projects
None yet
8 participants
@chinpei215
Member

chinpei215 commented Mar 3, 2016

This is a (multiple allowed):

  • bug
  • enhancement
  • feature-discussion (RFC)

What you did

I saw #8396 (diff)

Expected Behavior

The build should fail if an undefined value is used.

Actual Behavior

The build passed.

My Suggestion

I know Scrutinizer says that it's the best practice to use empty($value) instead of !$value. But I don't agree. Because empty($value) doesn't raise any notice even if $value is undefined. And it seemingly works on false conditions. As a result, we would make a bug like a #8396 sometimes.

I asked a member of Scrutinizer team why using empty() is the best practice. He said:

The main intention of this suggestion is to be explicit about the type conversion. One way to do that is using empty, another alternative is to add a cast like (bool)$results. The first one is a bit more expressive in terms of language, it reads if results empty which is what the code is checking, the other does not express the intention of the code as much.

But there are no such pros for non-English speakers.

He said also:

You are right that using empty would ignore undefined properties, but since you are using Scrutinizer, we would warn you about that, too :)

I re-asked:

Can Scrutinizer warn me that when I use if (empty($array['typo']))?

He didn't answer the question any more. But I am sure that Scrutinizer cannot warn it in such cases. In my opinion, heavy-using empty() doesn't make software robust. I think that we should use empty() only in the case that we really don't know whether the variable exists or not.

What do you think?

@chinpei215 chinpei215 added the RFC label Mar 3, 2016

@chinpei215 chinpei215 added this to the Future milestone Mar 3, 2016

@chinpei215 chinpei215 changed the title from RFC: Stop using slang as much as possible? to RFC: Stop using empty() as much as possible? Mar 3, 2016

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 3, 2016

Member

I totally mistook a title :D

Member

chinpei215 commented Mar 3, 2016

I totally mistook a title :D

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Mar 3, 2016

Member

I think you are right. That would help to prevent these hidden spelling issues.

Member

dereuromark commented Mar 3, 2016

I think you are right. That would help to prevent these hidden spelling issues.

@Phillaf

This comment has been minimized.

Show comment
Hide comment
@Phillaf

Phillaf Mar 3, 2016

Contributor

👍 Spotting these in phpunit is more robust than relying on scrutinizer.

Contributor

Phillaf commented Mar 3, 2016

👍 Spotting these in phpunit is more robust than relying on scrutinizer.

@htstudios

This comment has been minimized.

Show comment
Hide comment
@htstudios

htstudios Mar 3, 2016

Contributor

Restrict emtpy to be used against nullish (is there something better) or array index not existing?

Contributor

htstudios commented Mar 3, 2016

Restrict emtpy to be used against nullish (is there something better) or array index not existing?

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Mar 3, 2016

Member

What to do with optional arguments/options? You you need to call isset() first, which is almost the same as calling empty()

Member

lorenzo commented Mar 3, 2016

What to do with optional arguments/options? You you need to call isset() first, which is almost the same as calling empty()

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Mar 3, 2016

Member

I dont think he meant those cases, he only means the ones where we "know" that this variable or key must exist.

Member

dereuromark commented Mar 3, 2016

I dont think he meant those cases, he only means the ones where we "know" that this variable or key must exist.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 3, 2016

Member

@lorenzo That is the case we can use empty()/isset() as @dereuromark mentioned.

Member

chinpei215 commented Mar 3, 2016

@lorenzo That is the case we can use empty()/isset() as @dereuromark mentioned.

@htstudios

This comment has been minimized.

Show comment
Hide comment
@htstudios

htstudios Mar 3, 2016

Contributor

Can we formulate it positive; e.g use empty only on optional variables / keys? Is that it?

Contributor

htstudios commented Mar 3, 2016

Can we formulate it positive; e.g use empty only on optional variables / keys? Is that it?

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 3, 2016

Member

@jhli Probably, but I might not understand small nuance :p
I mean we shouldn't use empty() if the expression has no possibility to raise any notices (except for user's mistake).

Member

chinpei215 commented Mar 3, 2016

@jhli Probably, but I might not understand small nuance :p
I mean we shouldn't use empty() if the expression has no possibility to raise any notices (except for user's mistake).

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Mar 3, 2016

Member

@chinpei215 We could start small PRs to fix the obvious ones and go from there.

Member

dereuromark commented Mar 3, 2016

@chinpei215 We could start small PRs to fix the obvious ones and go from there.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 3, 2016

Member

@dereuromark OK. I would start to fix a typical example that using empty() against an existing property like a #8396. And note that I don't suggest to cause big changes in existing code. So I milestoned as Future :)

Member

chinpei215 commented Mar 3, 2016

@dereuromark OK. I would start to fix a typical example that using empty() against an existing property like a #8396. And note that I don't suggest to cause big changes in existing code. So I milestoned as Future :)

Phillaf added a commit to Phillaf/cakephp that referenced this issue Mar 3, 2016

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Mar 3, 2016

Member

I'm 👍 for not using empty() on properties. Like others I think it is still necessary to use array keys.

Member

markstory commented Mar 3, 2016

I'm 👍 for not using empty() on properties. Like others I think it is still necessary to use array keys.

@HavokInspiration

This comment has been minimized.

Show comment
Hide comment
@HavokInspiration

HavokInspiration Mar 3, 2016

Member

I'm also ok on the idea. I remember a few cases where using empty() caused troubles.

Member

HavokInspiration commented Mar 3, 2016

I'm also ok on the idea. I remember a few cases where using empty() caused troubles.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 4, 2016

Member

Thank you for your comments, everyone.

I am guessing that everyone also ok for not using empty() in other typical cases. I discovered good examples in the Cake\ORM\Entity class.

  1. Using empty($var) against a known variable - line 66
    The $properties will never be undefined because it is already defined at line 48. So we can use $properties instead of !empty($properties).
  2. Using empty($var['key']) against a known key of an array - line 58
    The $options['source'] will never be undefined because the key is already defined at line 55. So we can use $options['source'] instead of !empty($options['source']).

So let's start to stop using empty() in such typical cases, from next pull requests. I would close this RFC if there is no argument.

Thanks again for many comments!

Member

chinpei215 commented Mar 4, 2016

Thank you for your comments, everyone.

I am guessing that everyone also ok for not using empty() in other typical cases. I discovered good examples in the Cake\ORM\Entity class.

  1. Using empty($var) against a known variable - line 66
    The $properties will never be undefined because it is already defined at line 48. So we can use $properties instead of !empty($properties).
  2. Using empty($var['key']) against a known key of an array - line 58
    The $options['source'] will never be undefined because the key is already defined at line 55. So we can use $options['source'] instead of !empty($options['source']).

So let's start to stop using empty() in such typical cases, from next pull requests. I would close this RFC if there is no argument.

Thanks again for many comments!

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Mar 4, 2016

Member

@chinpei215 Those look like good opportunities to not use empty().

Member

markstory commented Mar 4, 2016

@chinpei215 Those look like good opportunities to not use empty().

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 Mar 4, 2016

Member

OK, then I think we reach a consensus on what we should do. Thanks everyone. Closing.

Member

chinpei215 commented Mar 4, 2016

OK, then I think we reach a consensus on what we should do. Thanks everyone. Closing.

@ravage84

This comment has been minimized.

Show comment
Hide comment
@ravage84

ravage84 May 23, 2016

Member

This consensus should be documented in the coding standard(s), otherwise it will get lost in the depths of our tickets.

http://book.cakephp.org/2.0/en/contributing/cakephp-coding-conventions.html
http://book.cakephp.org/3.0/en/contributing/cakephp-coding-conventions.html

A positive, exlusive or specific statement as @jhli mentioned should be used.

Member

ravage84 commented May 23, 2016

This consensus should be documented in the coding standard(s), otherwise it will get lost in the depths of our tickets.

http://book.cakephp.org/2.0/en/contributing/cakephp-coding-conventions.html
http://book.cakephp.org/3.0/en/contributing/cakephp-coding-conventions.html

A positive, exlusive or specific statement as @jhli mentioned should be used.

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 May 23, 2016

Member

I agree. I'll list examples shown in the coding standards. I am happy if someone can write the section instead of me by using my examples.

Member

chinpei215 commented May 23, 2016

I agree. I'll list examples shown in the coding standards. I am happy if someone can write the section instead of me by using my examples.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory May 23, 2016

Member

@chinpei215 I'm happy to help out with the words 😄

Member

markstory commented May 23, 2016

@chinpei215 I'm happy to help out with the words 😄

@chinpei215

This comment has been minimized.

Show comment
Hide comment
@chinpei215

chinpei215 May 23, 2016

Member

@markstory thank you 😣

He are the examples.

Definition
We use empty/isset when we are not sure whether the variable is defined or not.

Variable

function foo($var) // defined
{
    // not recommended, $var is already defined in the scope
    if (empty($var)) {
        // ...
    }

    // recommended
    if (!$var) {
        // ...
    }

    // recommended
    if ($var) {
        // ...
    }
}

Key

function foo(array $array)
{
    $array += [
        'key' => null, // defined
    ];

    // not recommended, the key is already set
    if (isset($array['key'])) {
        // ...       
    }

    // recommended
    if ($array['key'] !== null) {
        // ...       
    }
}

Property

private $property; // defined

public function foo()
{
    // not recommended, property is declared in the class
    if (!isset($this->property)) {
        // ...
    }

    // recommended
    if ($this->property === null) {

    }
}
Member

chinpei215 commented May 23, 2016

@markstory thank you 😣

He are the examples.

Definition
We use empty/isset when we are not sure whether the variable is defined or not.

Variable

function foo($var) // defined
{
    // not recommended, $var is already defined in the scope
    if (empty($var)) {
        // ...
    }

    // recommended
    if (!$var) {
        // ...
    }

    // recommended
    if ($var) {
        // ...
    }
}

Key

function foo(array $array)
{
    $array += [
        'key' => null, // defined
    ];

    // not recommended, the key is already set
    if (isset($array['key'])) {
        // ...       
    }

    // recommended
    if ($array['key'] !== null) {
        // ...       
    }
}

Property

private $property; // defined

public function foo()
{
    // not recommended, property is declared in the class
    if (!isset($this->property)) {
        // ...
    }

    // recommended
    if ($this->property === null) {

    }
}

markstory added a commit to cakephp/docs that referenced this issue May 23, 2016

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory May 23, 2016

Member

Pull request for the docs up now.

Member

markstory commented May 23, 2016

Pull request for the docs up now.

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