8
\$\begingroup\$

I have been programming for a while, but I've only made games and less serious stuff. I want to get more serious. I would like some feedback on this class that defines a person. Structure, readability, use of static variables, etc. Please point out everything that can be improved.

public class Person {

    private String firstName, lastName, address;
    private int age;
    private boolean hasAddress = false;
    private boolean hasAge = false;
    private static final String NO_ADDRESS = "No address defined";

    //Constructors

    //Minimum information required
    public Person(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
    }

    //All possible info
    public Person(String firstName, String lastName, int age, String address) {
        this(firstName, lastName);
        this.age = age;
        this.address = address;
        hasAge = true;
        hasAddress = true;
    }

    //No address defined
    public Person(String firstName, String lastName, int age) {
        this(firstName, lastName);
        this.age = age;
        hasAge = true;
    }

    //No age defined
    public Person(String firstName, String lastName, String address) {
        this(firstName, lastName);
        this.address = address;
        hasAddress = true;
    }

    //Methods

    //Get-methods
    public String getName() {
        return firstName + lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public int getAge() {
        if (hasAge)
            return age;
        else
            return 0; //Return 0 if no age is defined
    }

    public String getAddress() {
        if (hasAddress)
            return address;
        else
            return NO_ADDRESS;
    }

    //Set-methods
    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public void setAddress(String address) {
        this.address = address;
    }

}
\$\endgroup\$
  • 8
    \$\begingroup\$ You didn't test your code thoroughly before posting it here. If you had done that, you would have noticed that getName() just squashes together the first name and the last name, without a space in-between. This is probably not what you intended. \$\endgroup\$ – Roland Illig Jan 27 at 20:46
  • 9
    \$\begingroup\$ @RolandIllig For the purpose of getting a username, it actually could be. Anyways, I wouldn't consider the code as "not working" simply for that minor issue. \$\endgroup\$ – Simon Forsberg Jan 28 at 12:10
  • \$\begingroup\$ As designed you have a bug in your setAge and setAddress methods, you neglect to set your hasAge and hasAddress fields. if you start with a Person without a specified age and then call setAge subsequent calls to getAge will still return 0. \$\endgroup\$ – CaptianObvious Jan 28 at 13:23
  • \$\begingroup\$ I wouldn't have age as an attribute, since it depends on the time when it's queried. However, I would have a date of birth field and have a getAge() method that calculates the age in runtime and returns it. \$\endgroup\$ – luizfzs Jan 28 at 16:12
  • 1
    \$\begingroup\$ Classes are designed for a purpose. What is the purpose of modeling a person like this? \$\endgroup\$ – Eric Lippert Jan 28 at 20:42
13
\$\begingroup\$

Design

The problem with getName is already mentioned in the comments. firstName + lastName is at least missing a space. Note that there are multiple ways of creating a full name - you may want to leave the construction of a full name to the caller. For instance, Germans are a bit more formal and may prefer "lastname, firstname". Usually initials are also an important part of a name (and then there is title, junior/senior etc.).

Second is the age. An age is a property of a person, but it is not one that keeps constant in time. You generally store the birthday of a person. Usually the precise day is stored (YYYY-MM-DD), or in more complex systems just the month (YYYY-MM) or even just the year (YYYY) if the other information is unknown. This may seem strange, but not everybody knows their exact birthday. The age can then be calculated from that using comparison with the current date.

Related, using 0 to signify that no age is known is choosing a bad magic value (or sentinel). Magic values are already not recommended, but choosing a valid age for a baby as magic value is not a good idea at all.

Returning return NO_ADDRESS; is dangerous, especially if you don't have any method to indicate that NO_ADDRESS is indeed not an address. It is generally up to the caller to choose what to insert as string if no address is known, otherwise you may get packages posted to NO_ADDRESS. A hasAddress method would solve this. If you have an unknown address then you may use an Optional value instead. Accepting and returning an Optional<String> would remove the issue altogether and may bring down the number of methods and constructors. Storing the address as optional is not recommended because it cannot be serialized; probably best to use null, but make sure that you don't return null.

As you may notice, currently your Person instances can change completely by calling the setters. That's no good if you want to keep track of a person. This is why almost all organizations will create a unique ID for each person. That way any property can change without them becoming someone else. The ID can then represent the person in e.g. a transaction system.

Code

The code is generally well formed.

However there are some remarks to be made:

  • fields default to false so there is no need to initialize them to that value; it might be a good idea to set all fields in the various constructors so that they are not forgotten;
  • the if statement does not use braces; always use braces to subsequent changes do not result in unmaintainable code;
  • similarly, you will probably use class instances in collections, so you need to implement the hashCode and equals methods;
  • implementing toString is recommended for any class really.

Ideas

Using set methods is generally not a good idea. Having immutable objects often makes more sense. Other variants you may consider is having a Map of properties or having a factory / factory methods to create Person instances.

In Java, you may want to implement Serializable for data classes (and the static serializable UID - Eclipse will warn you about this) - that way you can more easily communicate data over a binary connection / stream. This is contended in the comments. In general, I would however try and keep in mind that you may want to be able to serialize a class in the future and - therefore - try and use serializable fields.

\$\endgroup\$
  • 5
    \$\begingroup\$ Optional is not meant to be used in fields. \$\endgroup\$ – Roland Illig Jan 28 at 5:43
  • 2
    \$\begingroup\$ Why should data classes be serializable? \$\endgroup\$ – Roland Illig Jan 28 at 5:46
  • 4
    \$\begingroup\$ And better than Optional<Integer>: OptionalInt. \$\endgroup\$ – Olivier Grégoire Jan 28 at 8:31
  • 1
    \$\begingroup\$ The only arg I've heard for optionals not being stored in fields is that they aren't serialization by Java's jank serialization mechanism. But the serialization mechanism has been so fraught with security vulnerabilities, it's better to just pretend it doesn't exist. \$\endgroup\$ – Alexander - Reinstate Monica Jan 28 at 13:21
  • \$\begingroup\$ "always use braces to subsequent changes do not result in unmaintainable code;" You can solve the same problem by just prohibiting multi-line if statements without blocks. if (condition) return; is totally fine IMO. You only get into dangerous goto fail land when you let the body go on a second line. Just don't do that \$\endgroup\$ – Alexander - Reinstate Monica Jan 28 at 13:24
7
\$\begingroup\$

Someone else already discussed this, but I would get rid of the boolean members hasAge and hasAddress. They don't really provide any information that the fields themselves don't provide.

An issue regarding the boolean flags is that you never update them, so they will be invalid once you use the setters. This is just a good demonstration of why you would be better off without them: they will eventually become inconsistent from the actual field state.

Instead of setting the address to a string saying no address available, just set it to an empty string or null and let the caller decide how to handle it. You could also add a hasAddress method that checks if the string is empty/null. An empty age could be signified as -1, assigned to a constant.

Also, instead of several different constructors, you could use the fluent builder pattern:

public class Person {

    private static final int EMPTY_AGE = -1;
    private static final String EMPTY_ADDRESS = "";

    private String firstName, lastName, address;
    private int age;

    public static class PersonBuilder {
        private String firstName = null, lastName = null, address = EMPTY_ADDRESS;
        private int age = EMPTY_AGE;
        public PersonBuilder firstName(String firstName) {
            this.firstName = firstName;
            return this;
        }
        public PersonBuilder lastName(String lastName) {
            this.lastName = lastName;
            return this;
        }
        public PersonBuilder address(String address) {
            this.address = address;
            return this;
        }
        public PersonBuilder age(int age) {
            this.age = age;
            return this;
        }
        public Person build() {
            Objects.requireNonNull(firstName);
            Objects.requireNonNull(lastName);
            return new Person(firstName, lastName, address, age);
        }
    }

    public static PersonBuilder builder() {
        return new PersonBuilder();
    }

    private Person(String firstName, String lastName, String address, int age) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.age = age;
        this.address = address;
    }

    public String getName() {
        return firstName + lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public int getAge() {
        return age;
    }

    public String getAddress() {
        return address;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public void setAddress(String address) {
        this.address = address;
    }

}

Now, you can build a Person like this:

Person.builder().firstName("John").lastName("Doe").age(25).address("1234 Some Road").build();

This is especially helpful when the class has more fields.

Besides being easier to read when constructing it, the builder pattern

  • Removes the possibility of mixing up constructor fields
  • Allows specifying the fields in any order
  • Guarantees the Person object returned will be valid. The build method will refuse to construct the object when required fields are missing. (This checking could alternatively be done in Person's constructor.)
\$\endgroup\$
  • \$\begingroup\$ Don't use sentinel values like -1, "". Arguably, you should even avoid null. Optional exists, and it fits the bill perfectly. What are the chances you'll use the age, forgetting that it could "not be set" (and poorly encoded as -1), and accidentally show a -1 year old age on the UI? Plus, what if this wasn't age, but some other value that could be negative? OptionalInt is a better choice. \$\endgroup\$ – Alexander - Reinstate Monica Jan 28 at 13:26
  • \$\begingroup\$ @Alexander-ReinstateMonica I completely agree, but apparently Java's Optional is not meant to be used for class fields. It also does not implement Serializable (unlike Guava's Optional which does). I think this was a poor choice on Java's part, but it is what it is. What would you suggest to denote the field is "empty" given that Optional isn't meant to be used for fields? \$\endgroup\$ – Nathan Jan 29 at 5:02
  • \$\begingroup\$ I guess in this case, the getters could return OptionalInt or Optional<String> and that would fit in with Optional's original intended use case. Though it still feels wrong to use sentinel values at all, as you pointed out. \$\endgroup\$ – Nathan Jan 29 at 5:10
  • \$\begingroup\$ Java's serialization mechanism was a poor choice on Java's part. It's fundamentally broken, and a consistent fountain of security vulnerabilities, so I would argue it shouldn't be used at all, ever, under any circumstances. So I don't think that's a good reason not to use Optional in args/fields. \$\endgroup\$ – Alexander - Reinstate Monica Jan 29 at 15:14
  • \$\begingroup\$ @Alexander-ReinstateMonica you mention the Serialization problem more than once, but in some cases serialization is required — clustering / load-balancing under a Tomcat application server, for example, requires all session attributes to be Serializable, and you may have reason to have a Person in the Session object… saying that serialization "shouldn't be used at all, ever, under any circumstances" is a non-starter, as there exist circumstances which require it. \$\endgroup\$ – Stephen P Jan 29 at 19:01
3
\$\begingroup\$

Instead of adding a boolean to check if the value is set, I suggest that you set the values with a default value; so if the value is not set, it will return it.

public class Person {
  private String firstName;
  private String lastName;
  private String address = "No address defined";
  private int age = 0;

//Constructors

  //Minimum information required
  public Person(String firstName, String lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
  }

  //All possible info
  public Person(String firstName, String lastName, int age, String address) {
    this(firstName, lastName);
    this.age = age;
    this.address = address;
  }

  //No address defined
  public Person(String firstName, String lastName, int age) {
    this(firstName, lastName);
    this.age = age;
  }

  //No age defined
  public Person(String firstName, String lastName, String address) {
    this(firstName, lastName);
    this.address = address;
  }

//Methods

  //Get-methods
  public String getName() {
    return firstName + lastName;
  }

  public String getFirstName() {
    return firstName;
  }

  public String getLastName() {
    return lastName;
  }

  public int getAge() {
    return age;
  }

  public String getAddress() {
    return address;
  }

  //Set-methods
  public void setFirstName(String firstName) {
    this.firstName = firstName;
  }

  public void setLastName(String lastName) {
    this.lastName = lastName;
  }

  public void setAge(int age) {
    this.age = age;
  }

  public void setAddress(String address) {
    this.address = address;
  }
}

If you want to keep the same logic, put -1 as the age and null to the address and compare the value in the getter.

public class Person {
   private String firstName;
   private String lastName;
   private String address = null;
   private int age = -1;
   private static final String NO_ADDRESS = "No address defined";

//Constructors

   //Minimum information required
   public Person(String firstName, String lastName) {
      this.firstName = firstName;
      this.lastName = lastName;
   }

   //All possible info
   public Person(String firstName, String lastName, int age, String address) {
      this(firstName, lastName);
      this.age = age;
      this.address = address;
   }

   //No address defined
   public Person(String firstName, String lastName, int age) {
      this(firstName, lastName);
      this.age = age;
   }

   //No age defined
   public Person(String firstName, String lastName, String address) {
      this(firstName, lastName);
      this.address = address;
   }

//Methods

   //Get-methods
   public String getName() {
      return firstName + lastName;
   }

   public String getFirstName() {
      return firstName;
   }

   public String getLastName() {
      return lastName;
   }

   public int getAge() {
      if (age > 0)
         return age;
      else
         return 0; //Return 0 if no age is defined
   }

   public String getAddress() {
      if (address != null)
         return address;
      else
         return NO_ADDRESS;
   }

   //Set-methods
   public void setFirstName(String firstName) {
      this.firstName = firstName;
   }

   public void setLastName(String lastName) {
      this.lastName = lastName;
   }

   public void setAge(int age) {
      this.age = age;
   }

   public void setAddress(String address) {
      this.address = address;
   }
}

\$\endgroup\$
  • \$\begingroup\$ one could also use Integer instead of int for age and default it to null as well \$\endgroup\$ – CaptianObvious Jan 28 at 13:25
  • 1
    \$\begingroup\$ Don't use sentinel values like -1, "", "No address defined". Arguably, you should even avoid null. Optional exists, and it fits the bill perfectly. What are the chances you'll use the age, forgetting that it could "not be set" (and poorly encoded as -1), and accidentally show a -1 year old age on the UI? Plus, what if this wasn't age, but some other value that could be negative? OptionalInt is a better choice. \$\endgroup\$ – Alexander - Reinstate Monica Jan 28 at 13:26
  • 1
    \$\begingroup\$ I have to disagree with Alexander — Use sentinels or unset values (null) when needed, but deal with them internally, and don't return them from getters. getAge() then could be public OptionalInt getAge() where if age < 0 then return OptionalInt.empty() and that leaves no chance of a caller accidentally using the sentinel values. I would tend to use null, so would have private Integer age = null; and getAge would return age == null ? OptionalInt.empty() : OptionalInt.of(age) \$\endgroup\$ – Stephen P Jan 29 at 19:13
2
\$\begingroup\$

I'm adding just some thoughts to explain how I would solve the problem: assuming in the simplest scenario you have two fields firstName and lastName that must be always compiled when you create a Person object then the simplest way is have to have a constructor that takes them as parameters:

public class Person {
    private String firstName;
    private String lastName; 

    public Person(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName  = lastName;
    }
    //getters and setters omitted for brevity
}

After you have two fields address and age that in one or more moments of the life of your objects can be null, to signal this situation to the user of your Person class you chose to return the values "No address defined" and 0 like the code below:

private static final String NO_ADDRESS = "No address defined";
public int getAge() {
    if (hasAge)
           return age;
    else
           return 0; //Return 0 if no age is defined
}
public String getAddress() {
    if (hasAddress)
           return address;
    else
           return NO_ADDRESS;
}

I disagree with your choice because the user of your class Person is obliged to know the value of String "No address defined" to identify which address is valid and which not with String.equals method and for me 0 is a valid value for age. My possible solution is the use of Optional and OptionalInt for address and age like my code below:

public class Person {
    private String firstName;
    private String lastName; 
    private String address;
    private Integer age; //<-- better use int age, see note for details

    public Person(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
    }

    public Optional<String> getAddress() {
        return Optional.ofNullable(address);
    }

    public OptionalInt getAge() {
        return OptionalInt.of(age);
    }

   //others setters and getters omitted for brevity
}

In this way the user of your class Person is forced to check if the fields address and age are set before using them:

Person person = new Person("firstName", "lastName");
person.setAddress("address");
person.setAge(10);
Optional<String> optAddress = person.getAddress();
if (optAddress.isPresent()) {
    System.out.println(optAddress.get());
}
OptionalInt optAge = person.getAge();
if (optAge.isPresent()) {
    System.out.println(optAge.getAsInt());
}

Note: following @Nathan's comment below, instead of use Integer for age field it is better to use directly int to avoid boxing and unboxing operations.

\$\endgroup\$
  • 1
    \$\begingroup\$ OptionalInt is meant to be used with a primitive int to avoid autoboxing and unboxing. \$\endgroup\$ – Nathan Jan 29 at 5:46
  • \$\begingroup\$ @Nathan Thanks, I included your comment in my answer. \$\endgroup\$ – dariosicily Jan 29 at 9:17
1
\$\begingroup\$

Something the other answer(s) haven't touched on.


Ternary Operators

getAge() and getAddress() can both be simplified using ternary operators:

public int getAge() {
    return age > 0 ? age : 0;
}

public String getAddress() {
    return address != null ? address : NO_ADDRESS;
}

Ternary works as following (in this case):

return (boolean condition) ? (value if true) : (value if false);

It's a shorter way to write the same code, and it looks a bit neater.

Also, always use brackets, even in a one line if/else. It made everything easier to read and understand, especially since you switch between the two in multiple places in your code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Note that ternary operators are very contentious, many people would rather see them gone from the language and some style guides disallow them. The bigger problem is to leave the "sentinel" values 0 and NO_ADDRESS in there in the first place, without warning. \$\endgroup\$ – Maarten Bodewes Jan 28 at 17:54
  • \$\begingroup\$ @MaartenBodewes What do you recommend to denote a field as empty? (Java's Optional isn't meant to be used for class fields) \$\endgroup\$ – Nathan Jan 29 at 5:43
  • \$\begingroup\$ We can simplify getAge() further to max(age, 0). In some languages, such as C# (but sadly Java's apparently not one of them), we could simplify getAddress() to address ?? NO_ADDRESS. \$\endgroup\$ – J.G. Jan 29 at 11:44
  • \$\begingroup\$ @MaartenBodewes Interesting. Is this Java-specific, or do lots of languages' communities have a debate like this? I mostly work in Python, where ternary operators are more legible, which might be part of why I'm unfamiliar with opposition to them. (The order of the arguments is different, which is annoying when transitioning languages, but that's another story.) \$\endgroup\$ – J.G. Jan 29 at 11:45
  • \$\begingroup\$ Yeah, it is all about legibility. I'll try and find some pointers later... \$\endgroup\$ – Maarten Bodewes Jan 29 at 14:46
1
\$\begingroup\$

As well as a missing separator, there's a missing null check here:

public String getName() {
    return firstName + lastName;
}

Don't fall into the trap of assuming that all people have a first name and a last name; some have only one of those. And some have different names in different contexts (e.g. pen-name/stage-name as well as a formal (passport) name).

Other code may be making assumptions that first name (or last name) is a "family name", but that's not certain either - read Falsehoods Programmers Believe About Names to pick up some other assumptions that might not be true.

\$\endgroup\$
  • \$\begingroup\$ That's a good point. If you do assume that a first name / last name always need to be present, then create a guard within the constructor to make sure they are not null. And yes, Java has terrible null handling, this class would be much neater in Kotlin. \$\endgroup\$ – Maarten Bodewes Jan 28 at 17:57
  • \$\begingroup\$ This is a good start but you can go a lot farther. What is Ludwig van Beethoven's last name? Is it "van Beethoven"? Would searching by last name for "Beethoven" then not find it? Is it "Beethoven"? If it is "Beethoven" then where does the "van" go? This notion that people have a first name and a last name that can then be concatenated to form a full name is simply false. \$\endgroup\$ – Eric Lippert Jan 28 at 20:45
  • \$\begingroup\$ And what about Kim Jong-un? Kim is his first name, and also his family name; in Korea, the family name is the first name, not the last name. Is that reflected in the model? Bizarrely enough, an official of the US government referred to him as "Chairman Un" recently. \$\endgroup\$ – Eric Lippert Jan 28 at 20:48
  • 2
    \$\begingroup\$ Yes, I ought to have linked to the falsehoods programmers believe about names from this answer (I complacently assumed that everybody is familiar with it, but that's obviously a falsehood itself). \$\endgroup\$ – Toby Speight Jan 29 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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