Problem
I’m looking for advice on implementing many-to-many. I’m modeling a site such as stack exchange: each Member has many Groups, each Group has many Members. I’ve introduce a Membership class. This class diagram summarized the code belowEventually there will be question and answer classes, but I want to get this part correct first.
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.ArrayList;
import java.util.List;
public class Member {
private LocalDateTime dateCreated;
private String firstName;
private String lastName;
private String screenName;
private String userID;
private List<Membership> memberships = new ArrayList<Membership>();
public Member(String firstName, String lastName, String screenName, String userID) {
super();
this.dateCreated = LocalDateTime.now();
this.firstName = firstName;
this.lastName = lastName;
this.screenName = screenName;
this.userID = userID;
}
public LocalDateTime getDateCreated() {
return dateCreated;
}
public String getFirstName() {
return firstName;
}
public String getLastName() {
return lastName;
}
public String getScreenName() {
return screenName;
}
public String getUserID() {
return userID;
}
protected void addMembership(Membership m) {
memberships.add(m);
}
public int getNumGroups() {
return memberships.size();
}
public List<Group> getGroups() {
List<Group> groups = new ArrayList<>();
for(Membership membership : memberships) {
groups.add(membership.getGroup());
}
return groups;
}
@Override
public String toString() {
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
String formatDateTime = dateCreated.format(formatter);
return "Member [dateCreated=" + formatDateTime + ", firstName=" + firstName + ", lastName=" + lastName
+ ", screenName=" + screenName + ", userID=" + userID + "]";
}
public static void main(String[] args) {
Member m = new Member("Les", "Wrigley", "Lesman", "lwrigley@gmail.com");
System.out.println(m);
}
}
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.ArrayList;
import java.util.List;
public class Group {
private LocalDateTime dateCreated;
private String title;
private String description;
private List<Membership> memberships = new ArrayList<Membership>();
public Group(String title, String description) {
super();
this.title = title;
this.description = description;
this.dateCreated = LocalDateTime.now();
}
public LocalDateTime getDateCreated() {
return dateCreated;
}
public String getTitle() {
return title;
}
public String getDescription() {
return description;
}
protected void addMembership(Membership m) {
memberships.add(m);
}
@Override
public String toString() {
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
String formatDateTime = dateCreated.format(formatter);
return "Group [dateCreated=" + formatDateTime + ", title=" + title + ", description=" + description + "]";
}
public int getNumMembers() {
return memberships.size();
}
public List<Member> getMembers() {
List<Member> members = new ArrayList<Member>();
for(Membership membership : memberships) {
members.add(membership.getMember());
}
return members;
}
public static void main(String[] args) {
Group g = new Group("Java Programming", "Questions related to programming in Java");
System.out.println(g.toString());
}
}
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
public class Membership {
private LocalDateTime dateJoined;
private Member member;
private Group group;
public Membership(Member m, Group g) {
dateJoined = LocalDateTime.now();
this.member = m;
this.group = g;
member.addMembership(this);
group.addMembership(this);
}
public LocalDateTime getDateJoined() {
return dateJoined;
}
public Member getMember() {
return member;
}
public Group getGroup() {
return group;
}
@Override
public String toString() {
String groupName = group.getTitle();
String memberName = member.getLastName() + ", " + member.getFirstName();
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
String formatDateTime = dateJoined.format(formatter);
String msg = String.format("Member:%s, Group:%s, Joined:%s", groupName, memberName, formatDateTime);
return msg;
}
public static void main(String[] args) {
Member m = new Member("Les", "Wrigley", "Lesman", "lwrigley@gmail.com");
Group g = new Group("Java Programming", "Questions related to programming in Java");
Membership membership = new Membership(m,g);
System.out.println(membership.toString());
}
}
Solution
public class Member {
private LocalDateTime dateCreated;
private String firstName;
private String lastName;
private String screenName;
private String userID;
private List<Membership> memberships = new ArrayList<Membership>();
How many of those fields should be mutable? I would have thought that at least dateCreated, userID and memberships should be final.
public Member(String firstName, String lastName, String screenName, String userID) {
super();
There’s no need to explicitly call super() for two reasons: firstly, the no-arg super-constructor is the default one; and secondly, the superclass doesn’t have any other constructors from which to distinguish.
this.firstName = firstName;
this.lastName = lastName;
this.screenName = screenName;
this.userID = userID;
Validation? firstName and lastName could reasonably be empty or null (see Falsehoods Programmers Believe About Names), but userID at the very least surely needs to be non-trivial.
Actually, that deserves to be a separate point. The treatment of names is simply wrong and will not survive contact with the real world without offending or confusing people whose names don’t fit into the pigeonhole of “one first name and one last name”. Not to mention that the usage given to those would be wrong if it assumes, for example, that the last name is a family name and more formal, whereas the first name is an individual name, and more intimate. Speaking as an expatriate, I have personal experience of the problems that can be caused by names from one culture not mapping well onto the naming schemes of another culture.
this.dateCreated = LocalDateTime.now();
This creates a few problems. How are you going to set up reliable test fixtures? Will you need to serialise and deserialise, and if so how are you going to do that? And are you guaranteed a consistent timezone? I looked at the Java docs for this, and found them rather ambiguous.
protected void addMembership(Membership m) {
memberships.add(m);
}
Do you have a specific use case in mind for which you will need to override this?
How about some validation that the membership is of the correct member?
public List<Group> getGroups() {
List<Group> groups = new ArrayList<>();
for(Membership membership : memberships) {
groups.add(membership.getGroup());
}
return groups;
}
Looking at this raises a few questions in my mind:
- Should the return value be immutable (
Collections.unmodifiableList) and documented as such, to prevent people changing the list in the belief that they’re changing the underlying model? - Why
List? Does the order matter? - Why not
Set? Is it possible for one member to have multiple memberships in the same group? Membershipcontains additional information: the date of association. How would I get at that information with this API?
public static void main(String[] args) {
Member m = new Member("Les", "Wrigley", "Lesman", "lwrigley@gmail.com");
System.out.println(m);
}
This is not a very useful test: about the only thing it verifies is that neither the constructor nor the toString() method throws an exception.
My comments on Group are virtually identical to those on Member.
public Membership(Member m, Group g) {
dateJoined = LocalDateTime.now();
this.member = m;
this.group = g;
member.addMembership(this);
group.addMembership(this);
}
...
public static void main(String[] args) {
Member m = new Member("Les", "Wrigley", "Lesman", "lwrigley@gmail.com");
Group g = new Group("Java Programming", "Questions related to programming in Java");
Membership membership = new Membership(m,g);
There is a general expectation that constructors will not have side-effects. However, it seems that the only way to add a member to a group is to call the Membership constructor. That will lead to code with statements which are just constructor invocations: new Membership(m,g); and which an incautious maintenance programmer might delete as unnecessary. If you care about OO purism then adding a member to a group should be a method of either Member or Group (or possibly both, with one passing the call on to the other). If you don’t, a static method Membership.create will communicate more clearly that it has side-effects.
@Override
public String toString() {
String groupName = group.getTitle();
String memberName = member.getLastName() + ", " + member.getFirstName();
If this is intended for use in log messages then it should use member.getUserID() since that’s unique. If it’s intended for UI messages then it should use member.getScreenName(), since that’s the whole purpose of screen names. I can’t see any scenario where member.getLastName() + ", " + member.getFirstName() is the best option.
I am not too sure about modeling the Membership as a first-class-citizen in Java. The only real information that is held there, is the dateJoined and your API does not even expose a method to query that.
Thus, I must assume, that Membership itself has no value as a “business class” but only as glue to map the underlying idea of a relational database model to Java classes. So, what you present in this solution is basically an ER-model, not a class model.
If all these assumptions are true, my advice is: don’t do this. Keep your business class model tied to the business layer, and leave the various needs to map this into an arbitrary kind of storage to the storage layer. Otherwise, you leak implementation details into your business code.
When you go for a storage layer at a later time, keep in mind that the popular object-relational mapping frameworks (JPA, Hibernate, …) will care about the relation table more or less automatically. On the other hand, if you chosse a no-sql database, a relation table does not make sense at all and you need a totally different database design.