Problem/Motivation

Coding standards and micro optimizations.

Proper use of array_key_exists() with isset() and empty() gains a speed improvement.
http://stackoverflow.com/questions/6884609/array-key-existskey-array-vs-...
http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

Proposed resolution

Remove all trailing whitespace from lines.
Replace array_key_exists() && !empty() with just !empty() because it implicitly does isset(), and is faster. If NULL is expected as a value use if (isset($array['key']) || array_key_exists('key', $array)) { @see references above.

Remaining tasks

Provide a patch.

User interface changes

API changes

CommentFileSizeAuthor
#5 coding_standards-2309321-5.patch26.17 KBfunc0der
#4 coding_standards-2309321-4.patch26.72 KBfunc0der
#1 code_clean_up-2309321-1.patch30.26 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new code_clean_up-2309321-1.patch30.26 KB

Here's a cleanup patch for whitespace and array_key_exists

func0der’s picture

func0der commented

Your patch is not appliable with git apply.
How did you create it?

henrijs.seso’s picture

henrijs.seso commented
Status: Needs review » Needs work

Re-roll is needed. See GIT instructions for proper steps.

func0der’s picture

func0der commented
Status: Needs work » Needs review
StatusFileSize
new coding_standards-2309321-4.patch26.72 KB

Re-done it. But I do not think using only "empty()" is a good practice.

Why again did u not use "isset()" and "empty()" combined? Has it been made sure that there is NO event where the used indexes are not set?
Applying a patch that fills up the watchdog with notices does not make so much sense, I think.

func0der’s picture

func0der commented
StatusFileSize
new coding_standards-2309321-5.patch26.17 KB

Since I did not use the current master, but the current dev release, I had one too many deletion.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@func0der, have a look at the issue summary for the reasons for that.

empty(), has a built in isset() call in it. It's good practice when it makes sense because it's fast and concise. You'd not use it if you expect a "0", 0, 0.0, false, array(), NULL @see http://ca2.php.net/empty

It's a concise version of !isset($var) || $var == false

What are you talking about notices in your watchdog?

Can you provide an interdiff on your patches there so I know what you changed? @see https://drupal.org/documentation/git/interdiff

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I just checked and my patch in #1 still applies against 7.x-2.x branch

func0der’s picture

func0der commented

Okay, where is the branch for this current downloadable version: https://www.drupal.org/node/1364484

I tried to apply your patch to that version and it did not work. That's why I made a new patch.
Your patch does apply to the branch. Mine not.
Your patch does not apply to the current released dev version, mine does.

that's strange :/
I am confused.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Run the following:

git clone --branch 7.x-2.x http://git.drupal.org/project/eck.git
cd eck
curl https://www.drupal.org/files/issues/code_clean_up-2309321-1.patch | git apply --index

  • fmizzell committed 31b3e53 on 7.x-2.x authored by func0der 
    Issue #2309321 by func0der, joelpittet: Eliminating trailing spaces from...
fmizzell’s picture

fmizzell commented
Title: Code Clean Up » Optimize code by using empty or isset instead of array_key_exists
Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Reviewed & tested by the community

@joelpittet thank you for pointing this out. Like @func0der I was skeptical about no PHP notices or warning being generated, but it is true that using empty or isset to check and array even with a non-existent key, will not produce anything. I committed the trailing space fixes, but decided not to commit the array_key_exists changes in 2.x as there is a logical difference between array_key_exists and !empty, and as we are getting closer to a stable release, I do not want to jeopardize that.

I will change the title of the issue and move it to be applied to the 3.x branch where we will have more time to look at each array_key_exists instance and determine whether a !empty should be used, and to deal with the consequences if our analysis is incorrect.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@fmizzell thanks for committing the whitespace cleanup.

It's a nice freedom to know you don't need to write both isset() && !empty() like you used to back in the day;) At least that's how I felt when I learned the trick was possible and that they included __isset check in the empty() function call implicitly.

As long as you can safely say you don't need NULL, [], 0, '0', '', FALSE... which is a lot of the time, !empty() check is quicker, easier to type and understand.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Status: Reviewed & tested by the community » Fixed

I guess this should be fixed since there is a commit. Doesn't really matter though, since 7.x-3.x is no longer supported.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.