6

I'm building a website that contains users with user profiles. Many of the fields in the profile are optional.

There is an opportunity for a lot of user-generated content, and so I need to display the author of this content in many different locations of the site (comments, posts, etc.). In the user's profile, he is able to (optionally) fill out his "first name", his "last name", and a "display name".

To display the author, I wrote a helper method that looks through a provided array of these fields and returns the most appropriate name for the user, in this order of preference:

  1. If the user filled out display_name, this will be displayed.
  2. If the user filled out first_name and last_name, but no display_name, it will display both names
  3. If the user only filled out first_name, it will display first_name.
  4. If the user only filled out last_name, it will display last_name.
  5. If all else fails, a user id will be displayed i.e. user123
  6. If none of the array keys are present, or the parameter is NULL, the name will display as NULL

The method works great, but it's ugly. There must be a way to beautify this with an alternative to nested if/else statements.

public function nameify($names = NULL) {
    $name = '';
    if (!empty($names)) {
        if (!empty($names['display_name'])) {
            $name = $names['display_name'];
        } elseif (!empty($names['first_name'])) {
            $name = $names['first_name'];
            if (!empty($names['last_name'])) {
                $name .= ' ' . $names['last_name'];
            }
        } elseif (!empty($names['last_name'])) {
            $name = $names['last_name'];
        }

        if (empty($name) && !empty($names['id'])) {
            $name = 'user' . $names['id'];
        } else {
            $name = 'NULL';
        }
    } else {
        $name = 'NULL';
    }
    return $name;
}
1
  • 3
    You just misuse return operator. It can be called not only at the end of the function but anywhere. And it will terminate further execution. Just like goto does. See x3ro's answer for the example Sep 28, 2010 at 16:00

9 Answers 9

8
public function nameify($names = NULL) {
    if ($names) {
        if (!empty($names['display_name'])) {
            return $names['display_name'];
        }
        if (!empty($names['first_name'])) {
            $name = $names['first_name'];
        } 
        if (!empty($names['last_name'])) {
            $name .= ' ' . $names['last_name'];
        }
        if (empty($name) && !empty($names['id'])) {
            $name = 'user' . $names['id'];
        }
    }
    return $name ? ltrim($name) : 'NULL';
}

Set the default first, and return that if nothing else matches. Then since we always want to return the display name if we have it do just that.

EDIT: Tweak to prevent returning "NULL "

7
  • Don't test for positive results and then go on inside the if statement, but test for "errors", and return if they appear. That way you reduce complexity and indentation.
    – fresskoma
    Sep 28, 2010 at 16:06
  • @x3ro in your own answer you started by adhering to your advice, but the second conditional does exactly what you say not to do.
    – Stephen
    Sep 28, 2010 at 16:09
  • Maybe I was a bit unclear in my comment, I apologize. I didn't mean to say that you shouldn't ever do that, but to avoid it, if possible. In my third statement I did it the opposite way, because I would've need more conditions in the if statement otherwise.
    – fresskoma
    Sep 28, 2010 at 16:14
  • 1
    With this solution, though very readable, if the user only enters a last name, the author is displayed as "NULL last_name"
    – Stephen
    Sep 28, 2010 at 16:21
  • Thats why null should never be a string ^^
    – fresskoma
    Sep 28, 2010 at 16:27
2

Using ternary conditions we can shorten and beautify the code:

public function nameify($names = NULL) {
    $name = 'NULL';

    if (!empty($names)) {

        $name = ($names['display_name']) ? $names['display_name'] : trim($names['first_name']." ".$names['last_name']);

        if(!$name) $name = ($names['id'] > 0) ? 'user'.$names['id'] : 'NULL';
    }

    return $name;
}
3
  • I don't believe so. I have warnings on and I don't get anything. It will just see it as a null value Sep 28, 2010 at 16:09
  • Oops. I meant a notice. here's from the php documentation Attempting to access an array key which has not been defined is the same as accessing any other undefined variable: an E_NOTICE-level error message will be issued, and the result will be NULL.
    – Stephen
    Sep 28, 2010 at 16:12
  • Ah, yes. I have my error level set to ignore notices. Seeing as the expected result is a NULL, the notice is simply informational and won't cause any problems (unless you have notices turned on which you wouldn't normally in a production environment) Sep 28, 2010 at 16:14
1

I would propose this:

public function nameify($names = null) {
    if(empty($names))
        return null;

    if(!empty($names['display_name']))
        return $names['display_name'];

    if(!empty($names['first_name'])) {
        $name = $names['first_name'];
        if (!empty($names['last_name'])) {
            $name .= ' ' . $names['last_name'];
        }
        return $name;
    }

    if(!empty($names['id]))
        return 'user' . $names['id'];

    return null;
}
0

It is not much, but because $name it is at least NULL:

public function nameify($names = NULL) {
    $name = 'NULL';
    if (!empty($names)) {
        if (!empty($names['display_name'])) {
            $name = $names['display_name'];
        } elseif (!empty($names['first_name'])) {
            $name = $names['first_name'];
            if (!empty($names['last_name'])) {
                $name .= ' ' . $names['last_name'];
            }
        } elseif (!empty($names['last_name'])) {
            $name = $names['last_name'];
        }

        if ($name=='NULL' && !empty($names['id'])) {
            $name = 'user' . $names['id'];
        } 
    } 
    return $name;
}
0
//pointers to functions
$arrayOfSulutions{"display_name_strategy", "full_name_strategy" ..., "null_strategy" } 
function display_name_strategy{
     return $names['display_name'];
}
$i = 0;
while($res == null){
     $res = call($arrayOfSulutions[$i++]);
}
9
  • @Stephen: These is solution from Java world) Sep 28, 2010 at 16:01
  • -1 Because of two things: Maximum complexity (== maximum debugging time) and minimum performance (call_user_func is extremely slow in PHP)
    – fresskoma
    Sep 28, 2010 at 16:04
  • @x3ro : minimum performance - yes. complexity - no. Flexibility- yes! Sep 28, 2010 at 16:08
  • @Stas: Imho, this is way more complex than an if/else solution. Anything you have to think about more than 5 seconds to understand the basic logic is complex for such a task. Also, the author didn't mention any needs of flexibility.
    – fresskoma
    Sep 28, 2010 at 16:12
  • 1
    I don't agree. The first think you should think about when writing your code is maintainability and readability! When you write a flexible solution only you can mantain, that won't help anyone at all.
    – fresskoma
    Sep 28, 2010 at 16:26
0

Somewhat less readable, but effective):

list($idx,$name) = array_shift(array_filter(array(
    $names['display_name'],
    implode(' ',array_filter(array($names['first_name'],$names['last_name']))),
    'user'.$names['id'];
    )));
4
  • -1 "Somewhat less readable" ... Seriously, who is going to debug something like that... This is why I hate PHP...
    – fresskoma
    Sep 28, 2010 at 16:08
  • Please don't misunderstand my comment though, its an neat idea, its just nothing anyone should ever use in production unless there is any other way to do it ;)
    – fresskoma
    Sep 28, 2010 at 16:15
  • Bah, I love it. It could be made more readable but this is a beautiful example of some of the hidden parts of php.
    – Chuck Vose
    Sep 28, 2010 at 16:29
  • 1
    @x3ro : I agree this is something I'd never use in production, just had to throw an if-less solution out there after the multitude of all not-quite-that-much-better answers. For educational purposes only, and I'd hate it if a coworker tried to use similar constructs in production :)
    – Wrikken
    Sep 28, 2010 at 16:46
0

A State machine works very nicely for involved logic like that. It's very simple to implement as well (using a switch statement).

3
  • Do you have an example of how to implement this abstraction for my problem? Even if it's in pseudo code I'd like to see it.
    – Stephen
    Sep 28, 2010 at 16:07
  • Although I've accepted Chuck Vose's answer, I'm still interested in this one.
    – Stephen
    Sep 28, 2010 at 17:35
  • static enum { sleep, work, SurfStackOverflow, StareOutWindow, Game } state = sleep ; ClockChimeEvent() { switch (state) { case sleep: if ( hour == 9 ) { state = SurfStackOverflow; } break; case SurfStackOverflow: if ( hour == 10 ) { EatBreakfast(); state = work; } break; case work: if ( hour == 12 ) { EatLunch(); state = StareOutWindow ; } break; case StareOutWindow : if ( hour == 17 ) { EatDinner(); state = Game ; } break; case Game : if ( hour == 23 ) { EatSnack(); state = sleep ; } break; } }
    – Jay
    Sep 29, 2010 at 17:08
0

I'm not sure that my version would be simplier, but here it is:

public function nameify($names = null) {
    $result = array();

    if( !empty($names['display_name']) ) {
        array_push($result,$names['display_name']);
    } else {
        if( !empty($names['first_name']) ) {
            array_push($result, $names['first_name']);
        }
        if( !empty($names['last_name']) ) {
            array_push($result, $names['last_name']);
        }
    }

    if( empty($result) && !empty($names['id']) ) {
        array_push($result, 'user'.$names['id']);
    }

    return (empty($result) ? 'NULL' : implode(' ', $result));
}
0

I'd go with:

if( empty($names['display_name']) ) {
    $name = $names['first_name'] . ' ' $names['last_name'];
else
    $name = $names['display_name'];

$name = trim($name);
if( empty($name) ) 
    $name = 'user'.$names['id'];
if( empty($name) ) 
    $name = 'NULL';

This would be the 'core logic'... there will need to be other checks like $names != NULL or something..

3
  • 1
    I think you mean empty and not !empty on line one.
    – Stephen
    Sep 28, 2010 at 15:56
  • @Stephen right you are. Will edit. Please feel free to edit others' answers.
    – jrharshath
    Sep 28, 2010 at 18:39
  • Would if I could. You need 2000 rep to do that.
    – Stephen
    Sep 28, 2010 at 18:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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