String concatenation of name parts

Question

I have written the following function to concatenate the parts of a name to produce the full name:

/**
 * Returns the full name of the Person.
 * @param boolean $includeTitle Whether to include the Person's title.
 * @param boolean $includeMiddleNames Whether to include the Person's middle names.
 * @param string $separator Separator character.
 * @return string Full name of the Person.
 */
public function getFullName($includeTitle = false, $includeMiddleNames = false, $separator = ' ') {
  if ($includeTitle && $this->getTitle()) {
    $name = $this->title;
  } else {
    $name = '';
  }
  if ($this->forename) {
    if ($name) {
      $name .= $separator;
    }
    $name .= $this->forename;
  }
  if ($includeMiddleNames && $this->middlenames) {
    if ($name) {
      $name .= $separator;
    }
    if ($separator != ' ') {
      // multiple middle names will be separated by spaces and so will need to be converted to $separator
      $name .= str_replace(' ', $separator, $this->middlenames);
    } else {
      $name .= $this->middlenames;
    }
  }
  if ($this->surname) {
    if ($name) {
      $name .= $separator;
    }
    $name .= $this->surname;
  }
  return $name;
}

I changed it to use an array and implode the parts - which has made it less lines, but I'm not really sure if it is written better as a result:

$nameParts = array();
if ($includeTitle && $this->getTitle()) {
  $nameParts[] = $this->title;
}
if ($this->forename) {
  $nameParts[] = $this->forename;
}
if ($includeMiddleNames && $this->middlenames) {
  if ($separator != ' ') {
    $middlenames = str_replace(' ', $separator, $this->middlenames);
  } else {
    $middlenames = $this->middlenames;
  }
  $nameParts[] = $middlenames;
}
if ($this->surname) {
  $nameParts[] = $this->surname;
}
return implode($separator, $nameParts);

I'm sure there must also be a better way but I can't think what. Any suggestions?

Answer 1

I assume the class this method resides in represents a person's full name (and title).

Two remarks before suggesting major refactorings:

• You are using a getter getTitle and direct access to $this->title: stick for one. Usually I prefer to use getters within the class as well (as usual they contain some logic beside returning). Your other variables don't have getters at all. Also, getting a value just for checking for its existence is bad practice. Introduce some existence method for this (e.g. hasTitle).
• Don't store information serialized during runtime ($middlenames). As you obviously need the middle names as an array, store them accordingly.

Now looking at your class, it has two responsibilities: (1) representing a name, and (2) formating a name. Moving the name formatting into a separate class decouples representation and formating. It allows you to have different means to format at name at the same time and when you want to change the formatting you don't have to touch the name class at all. This is how such a formatter might look like (untested):

class NameFormater {
    private $seperator;
    private $includeTitle;
    private $includeMiddleNames;

    public function __construct($seperator, $includeTitle, $includeMiddleNames) {
        // argument validation here
        $this->seperator = $seperator;
        $this->includeTitle = $includeTitle;
        $this->includeMiddleName = $includeMiddleName;
    }

    public function format(Name $name) {
        $nameParts = array();

        if ($this->includeTitle && $name->hasTitle()) {
            $nameParts[] = $name->getTitle();
        }
        if ($name->hasForName()) {
            $nameParts[] = $name->getForName();
        }
        if ($this->includeMiddleNames && $name->hasMiddleNames()) {
            $nameParts = array_merge($nameParts, $name->getMiddleNames());
        }
        if ($name->hasLastName()) {
            $nameParts[] = $name->getLastName;
        }

        return implode($this->seperator, $nameParts);
    }
}

Now this method could be refined further. E.g. we first could allow $nameParts to contain null values if the respective part was not set and filter them before implodeing. This would make it more challenging to read though and I prefer a bit more verbosity over compactness. The current version is trivial to follow and understand. This is due to no complex nesting or logic and names that speak for themselves (e.g. hasTitle) - and in my opinion this is the most important point in programming :)