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?

• Simon Forsberg: Welcome to Code Review! Just wanted to tell you that it's a nice little first-post you have here. It seems like you have understood exactly what this site is about.

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 :)

• user37349: Thanks for the feedback. I was using the get methods but realised I didn't want to use their logic so changed to getting them directly - guess I missed one. I agree, the has methods do read better. I will start using then from now on. I'm not sure I really see the benefit in having the NameFormatter class. I have standard get/set methods, then additional methods such as getFullName, getFullLegalName (which just calls getFullName, passing the relevant parameters), etc. and finally methods for db interaction – load, save, etc. Would you put these methods into different classes?
• Fge: Yes I definitely would. One class represents the object you want to model (your domain model/entity, in your case the name). When deciding about what is part of your model, and what not it sometimes helps to imagine "being the object" you want to model. As a name, I don't care how I am formatted or how I am stored. Separating your concerns really helps to keep your classes small, makes changes to a class less likely, and easier to read.
• user37349: This has opended up a whole can of worms! So I've posted a related topic: codereview.stackexchange.com/questions/42599/….