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;
}
Stephen
  • 18,827
  • 9
  • 60
  • 98
  • 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 – Your Common Sense Sep 28 '10 at 16:00

9 Answers9

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 "

Chuck Vose
  • 4,560
  • 24
  • 31
  • 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 '10 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 '10 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 '10 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 '10 at 16:21
  • Thats why null should never be a string ^^ – fresskoma Sep 28 '10 at 16:27
  • Thanks Stephen, you're right. I've tweaked a little bit to work around that. – Chuck Vose Sep 28 '10 at 16:28
  • Great! I was doing something with a ternary mixed with your solution and it looked just like this. Thanks! – Stephen Sep 28 '10 at 16:30
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;
}
Brendan Bullen
  • 11,607
  • 1
  • 31
  • 40
  • I don't believe so. I have warnings on and I don't get anything. It will just see it as a null value – Brendan Bullen Sep 28 '10 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 '10 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) – Brendan Bullen Sep 28 '10 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;
}
fresskoma
  • 25,481
  • 10
  • 85
  • 128
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;
}
CristiC
  • 22,068
  • 12
  • 57
  • 89
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..

jrharshath
  • 25,975
  • 33
  • 97
  • 127
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++]);
}
Stan Kurilin
  • 15,614
  • 21
  • 81
  • 132
  • @Stephen: These is solution from Java world) – Stan Kurilin Sep 28 '10 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 '10 at 16:04
  • @x3ro : minimum performance - yes. complexity - no. Flexibility- yes! – Stan Kurilin Sep 28 '10 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 '10 at 16:12
  • @x3ro : It's only your IMHO, so.. I think that we must think about flexebility and readable at first, than about performance (In standard situations) – Stan Kurilin Sep 28 '10 at 16:18
  • 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 '10 at 16:26
  • @x3ro : I thing these is topic to holly war) We worked with different projects, have different experience, read different books and so on, so we have different answers about priorities in software. – Stan Kurilin Sep 28 '10 at 16:32
  • @Stas: Yep, but its like that with almost any programming related topic :D Really, I wasn't trying to offend anyone... I mean, no one can really be objective on such matters, because we're all influenced by our experience :) – fresskoma Sep 28 '10 at 16:35
  • @x3ro : I thing that our last two post's will be happy end of our discussion) – Stan Kurilin Sep 28 '10 at 16:39
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'];
    )));
Wrikken
  • 69,272
  • 8
  • 97
  • 136
  • -1 "Somewhat less readable" ... Seriously, who is going to debug something like that... This is why I hate PHP... – fresskoma Sep 28 '10 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 '10 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 '10 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 '10 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).

Jay
  • 13,803
  • 4
  • 42
  • 69
  • 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 '10 at 16:07
  • Although I've accepted Chuck Vose's answer, I'm still interested in this one. – Stephen Sep 28 '10 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 '10 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));
}
andreyv
  • 89
  • 5