Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is the pattern I've been using for "If the variable is empty, set a default value", and for variables like $muffin it has all seemed well and good. But in the following real example this pattern gives me a super long line which smells a bit to me. Is there a cleaner way to do this?

$newsItems[0]['image_url']= $newsItems[0]['image_url']=='' ? '/img/cat_placeholder.jpg' : $newsItems[0]['image_url'];

maybe:

$newsItems[0]['image_url']= $newsItems[0]['image_url']=='' ? 
    '/img/cat_placeholder.jpg' : 
    $newsItems[0]['image_url'];   //this doesn't look much better to me?
share|improve this question
add comment

4 Answers

up vote 14 down vote accepted

Before dealing with your question, I'd like to point out that you probably want to use empty() instead of comparing with an empty string and that if you really want to compare to an empty string, use the === operator.

Anyway, default initialization is a real problem in many languages. C# even has a ?? operator to solve this kind of issue! So, let's try to make it better to make your code better step by step.

Ternary operator no more

The one-liner is too long, and using the ternary operator on multiple lines is a bad idea: that's what if-statement are for:

if (empty($newsItems[0]['image_url'])) {
    $newsItems[0]['image_url'] = '/img/cat_placeholder.jpg';
}

This is more readable for two reasons : it's easier to understand the syntax, but more importantly, it's easier to see that you simply want a default value when there's nothing. We don't need the 'else' part which is confusing with the ternary operator.

Note: as mentioned by Simon Scarfe and corrected by mseancole, PHP also has some special ternary syntax to do this:

$newsItems[0]['image_url'] = $newsItems[0]['image_url'] ?: 'img/cat_placeholder.jpg';

Factorize it!

If you're doing this only once or twice, then all is good, but otherwise you'd want to factorize it into a function, since you don't repeat yourself, right? The simple way is:

function default_value($var, $default) {
    return empty($var) ? $default : $var;
}

$newsItems[0]['image_url'] = default_value($newsItems[0]['image_url'], '/img/cat_placeholder.jpg');

(The ternary operator does make sense here since variable names are short and both branches of the condition are useful.)

Still too long

Can we do better? Yes we can! We're writing the variable name twice, but passing by reference can help us here:

function default_value(&$var, $default) {
    if (empty($var)) {
        $var = $default;
    }
}

default_value($newsItems[0]['image_url'], '/img/cat_placeholder.jpg');

It's much shorter, and feels more declarative: you can look at a bunch of default_value calls and see what the default values are instantly.

share|improve this answer
 
Would like to point out that the ternary shorthand in this example is wrong. The shorthand reads as follows: ifTRUE ?: ifFALSE so what is actually happening is you're overwritting the information already in that array field and leaving it blank if empty. So should be changed to not empty check. Would also like to mention, this only works on PHP >= 5.3 –  mseancole Jul 16 '12 at 13:21
 
Thanks, edited. (I hope it's correct now...) –  Quentin Pradet Sep 13 '12 at 9:33
add comment

From http://us2.php.net/manual/en/language.operators.comparison.php#language.operators.comparison.ternary :

Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise.

So could you write?:

$newsItems[0]['image_url'] = $newsItems[0]['image_url'] ?: '/img/cat_placeholder.jpg';
share|improve this answer
1  
This is one of my favorite things to write in PHP. It feels elegant and naughty at the same time! :) I normally only use it when assigning to another variable or return from a function, though. –  David Harkness Jul 15 '12 at 2:06
add comment

Doing it OOP style

If you are doing OOP you could also reconsider if using a plain array is the way to go. I guess 'image_url' is not the only attribute of your news items. Convert this array structure to an object (cf. http://sourcemaking.com/refactoring/replace-array-with-object). In further consequence $newsItems may get changed to an array of NewsItem instances.

'/img/cat_placeholder.jpg' could be the default value of the attribute 'image_url' in your new class NewsItem. Either your client code decides wether to set a new value (e.g. based on being !empty()) or your NewsItem class has a setter method for the attribute 'image_url' that automatically sets the default value if it gets an empty string.

share|improve this answer
add comment

The extract() function http://www.php.net/extract can also help.

It doesn't work for individual array keys like you have here, but it could be useful in other circumstances.

extract(array('foo'=>'bar'), EXTR_SKIP);

This will assign bar to $foo as a default value only if it's not already set. If $foo is already set it will skip it. You can include multiple variables in the array.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.