Java OOP principes

Question

we have homework in Java to do some users, contracts, and some API. We should create a user with some information about him and one of that information should be address (one where he live and one where post should come) But I am not sure if I did it correctly.

I create a package called user inside which i created User.class

package xxx;

import java.util.List;

public abstract class User {

    private int ID;
    private String firstName;
    private String lastName;
    private int idNumber;
    private String email;
    private String officialAddress;
    private String postAddress;
    private List contracts;



    public User(String firstName, String lastName, int idNumber, String email) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.idNumber = idNumber;
        this.email = email;
    }
    public String getFullName() {
        return firstName + " " + lastName;
    }


}

and OfficialAddress.class

package xxx;

public class OfficialAddress extends User {

        private int zipcode;
        private String region;
        private String streetName;
        private int streetNumber;

    public OfficialAddress(String firstName, String lastName, int idNumber, String email) {
        super(firstName, lastName, idNumber, email);

    }
}

I just want to ask if is good practice to do it like this or if it should be different. We just cant use nested classes and static methods. Thanks for review.

Answer 1

Welcome to Code Review and thanks for sharing your code.

Your class OfficialAddress extends your class User. This does not look right.

By extending a class you declare an is a relationship. But an address never is a user.

instead a user has a Address. In OOP this is expressed by giving the owner (class User) a property of the other class (OfficialAddress):

public class OfficialAddress { // no extending here
  // ...
}

public class User{
  private OfficialAddress officialAddress;
  // ...
  public User(String firstName, String lastName, int idNumber, String email, OfficialAddress, officialAddress) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.idNumber = idNumber;
        this.email = email;
        this.officialAddress = officialAddress;
    }
} 

Answer 2

When designing a class never ignore what you're making the construction and using code look like.

User user = new User(
    123,
    new FullName("Homer", "Jay", "Simpson"),
    new Email("[email protected]"),
    new Address("742", "Evergreen Terrace", "??", "?????"),
    new Contacts(124, 125, 126, 127)
);

user.sendBill(overdueNotice);
user.sendEmail(promotion42);

This uses composition, not inheritance, which, without a good reason otherwise, is generally preferred.

Done this way the class can be fully immutable since no setters have been used. This means it can be shared across threads without fear of using it while it's partially updated.

Breaking the class into smaller classes avoids long lists of unreadable constructor parameters.

Answer 3

In User, you define a user's name as:

private String firstName;
private String lastName;

This is something that is very common, and very wrong. I am one of those oddballs that use a first initial, a middle name, and a last name. This is not uncommon in the US, and in my case is a family tradition -- we start using our middle name when we become adults, and so it feels like somebody is calling me a child when they insist on using our first name. And no, I will not put my middle name into a slot that asks for a first name.

It can get even stranger even in the US. We had "The Artist Formerly Known As Prince" (look it up if you don't remember) -- he changed his name to an unpronounceable symbol.

Then there was "Moon Unit Zappa" (Moon Unit was her first name).

Then there are names from countries/cultures outside the US.

And, just to make it even nastier, names are not unique! Even with a single "name" field, I doubt there are more than a handful of unique names in the US (Ms. Zappa may be one of those unique names, I certainly hope so.)

Please check out this link: Falsehoods Programmers Believe About Names

Answer 4

A few remarks:

• you have an ID and idNumber;
• the constructor of User doesn't initialize a lot of fields, better make them Optional<String> or Optional<Address> if they are;
• contracts is a List without type parameter (e.g. List<Contact> or List<User> would be logical type parameters;
• white space and indentation are a bit funky (sometimes no empty lines, sometimes up to three, what's up with that?).

I'm not sure that contacts is something that is inherent to or part of a user. So I might want to create that relationship using e.g. a table rather than making it a field of a user.

I agree with Timothy, an official address is clearly a "has a" relationship. However, is an official address of another type as a PostAddress? I'd at least make Address an interface or abstract class. Or it could a full blown class if there are no real differences between the two kind of addresses.

When assigning an address within the constructor you may want to make the address immutable or otherwise make a copy. Otherwise you are not correctly implementing data encapsulation. The caller can change the given address and the user class will change with it. Same goes for any getOfficialAddress getter: either use an immutable officalAddress or perform a copy / clone.