CODEWITHSUNDEEP
phpif-statementcode-duplication
Dan
Dan

Reputation: 16256

PHP - refactoring this if statement to avoid duplication

In this snippet of code we type $inputs['user_id'] 3 times.

if (isset($inputs['user_id']) && $inputs['user_id']) { // The consumer is passing a user_id
    doSomethingWith($inputs['user_id']);
}

What's the most readable and robust refactoring I can do to avoid the duplication and avoid any notice that the index user_id doesn't exist?

Thanks.

Upvotes: 3

Views: 181

Answers (4)

i--
i--

Reputation: 4360

Below might not be the best way for every situation, but definitely cuts down on the repetition.

Your example code would turn into:

doSomethingWith($inputs['user_id']);

and your function would look like this (notice the argument supplied by reference, to avoid the undefined variable warning):

function doSomethingWith(&$userID) {
   if (empty($userID)) return;
   // ... actual code here ...
}

Upvotes: 1

hek2mgl
hek2mgl

Reputation: 158090

What about this:

// put validation check to the function body
function doSomethingWith($userId) {
     if($userId === -1) {
         // if this is not a valid user id -> return
         return;
     }
     // do something ...
}

// initalize $user with proper default values.
// doing so you can be sure that the index exists
$user = array(
    'id' => -1,
    'name' => '',
    ...
);

// merge inputs with default values:
$user = array_merge($user, $request);

// now you can just pass the value:
doSomethingWith($user['id']);

Upvotes: 1

shadyyx
shadyyx

Reputation: 16055

Here nothing is wrong with the duplication. You cannot assign $inputs['user_id'] to a variable before checking if it is set, otherwise this will produce a Notice undefined index ....

The only thing here could be done is to omit the isset call and use !empty instead, like this:

if(!empty($inputs['user_id'])) {
    doSomething($inputs['user_id']);
}

Now You are only typing it twice and the check 

!empty($inputs['user_id'])

equals to 

isset($inputs['user_id']) && $inputs['user_id']

EDIT: based on a comments, here is a quote from documentation:

The following things are considered to be empty:

"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)
$var; (a variable declared, but without a value)

So either empty(0) or empty('0') will return true, that means 

if(!empty('0') || !empty(0)) { echo "SCREW YOU!"; }

will echo nothing... Or, in polite way, I will repeat the statement above:

!empty($inputs['user_id']) === (isset($inputs['user_id']) && $inputs['user_id'])

EDIT 2:

By omitting the isset and replacing by !empty the variable is still checked, whether the index is already set, please read the documentation, which says:

No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.

Upvotes: 4

Daniele Vrut
Daniele Vrut

Reputation: 2873

Assuming that 0 and "" and null are not valid user_ids:

if ($id = $inputs['user_id']) { 
    doer($id);
}

YOu can also do with evil @ to avoid notice in your logs, (I don't like this way):

if ($id = @$inputs['user_id']) { 
    doer($id);
}

Upvotes: 0

Related Questions