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.

• Basil Bourque: Your title is weak. Rewrite to summarize your specific technical issue.

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;
 }
} 

• user223904: Thank you very much it helps me a lot.
• chrylis -cautiouslyoptimistic-: @JurajJakubov There is a name for this guideline: Prefer composition over inheritance.
• Timothy Truckle: @chrylis-onstrike- while this guideline exists it is not applicable here because inheritance is the completely wrong approach here, not even an option. PCoI is about providing common behavior while the classes in Questioners example do not have anything in common.

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

• Mast: Considering this is a homework question, I think the onus is on the specification. Not on the code.
• NomadMaker: He asked in Code Review, I told him what I thought would improve his code. And yes, this is probably due to a poor specification, and maybe this will get back to them.

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.

• user223904: I never thought about details like whitespaces. Thank you for your effort.
• Timothy Truckle: @JurajJakubov most IDEs provide an auto formatter feature (in eclipe you apply it with <ctrl><shift><f>), just use that as often as possible.
• Olivier Grégoire: "better make them Optional<String> or Optional<Address> if they are" and then your IDE complains that you have Optional fields. No. Don't make them optional in the field, make their accessor (getter) return an optional and return Optional.ofNullable(field).
• Maarten Bodewes: If you must. If I remember it is just a problem with serialization. As long as null is not returned.
• Frank Hopkins: @JurajJakubov the formating function can also be set to be applied automatically and once you work in a team with a version control software formating does get important as you want to avoid code changes just because of spacing changes that make it hard to track the actually important changes in your version control history.