Should all data classes in our TP be immutable?

[honganhcs]

I noticed that the Person class and the classes representing its fields all have their instance members declared as final. Thus, for EditCommand, the execute method creates a new Person object, which replaces the person-to-edit object in the UniquePersonList. What are the benefits of doing this, compared to modifying the fields in the person-to-edit object?

[takufunkai]

Imo, this makes the program (and its classes) a lot simpler to reason about:

• No need for setters
  • Rely on our method of construction to build the object correctly
• No need for extra validation
  • With setters, we need to run validation checks for each of the possible update methods

To some extent I think this is DRY and reduces code complexity

[YiHern-Lee]

I agree with @takufunkai. In the future when the program grows and there is a need to share references to the same object, allowing the mutation of objects may result in unwanted side-effects and make debugging more difficult.

[TypeDefinition]

I disagree that Person should be immutable. Certain granular things makes sense to be immutable, such as Phone, Address, Name etc.

However, the entire core of our program revolves around modifying a person. In the real world, if a person changed their phone number, they'd replace my entire phone number. But we wouldn't replace the entire person with someone else entirely.

I find that by needlessly making everything immutable, we end up with silly classes like EditPersonDescriptor. Combined with the fact every field in Person is so highly coupled, we end up with a problem where simply adding a new field like Remark requires changes to an unreasonably large number of files, and is frankly, unmaintainable and much more bug prone than to have a simple, mutable design from the start.

TLDR: Immutability should be applied when it fits the situation. But not when it conflicts with the core purpose of the application.

[honganhcs]

Thank you for all your inputs! I realised that one reason why editing a person requires replacing the Person object in the UniquePersonList is so that listeners to the ObservableList will know that there is an update to the list. I think that listeners to ObservableLists only listen to changes in the list and not to changes in an item in the list. May I know if my understanding is correct Prof @damithc?

[damithc]

May I know if my understanding is correct Prof @damithc?

@honganhcs most likely so. I can't confirm as I wasn't heavily involved in writing the code.

[damithc]

Glad to see this kind of discussions on design decisions. Do keep it up as this is an interesting topic.

In general, we encourage you to question design decisions in the current code and explore alternatives. The work done when exploring alternatives can earn credit (as they require effort) and also can be submitted for CS2103R.

Things to keep in mind:

1. Overuse or blind application of principles and best practices is not good. Everything has pros and cons, and some have trade-offs/conflicts with each other. But they are not totally useless either (i.e., don't be hasty when discarding them)
2. Keep discussions civil and professional. Don't be too dogmatic or condescending of others' views.
3. Keep an open mind when critiquing existing code. Often there is more to it than meets the eye at first. Some past decisions may have been misguided to begin with, or justified at the time but no longer so due to subsequent evolution of the code (e.g., AB3 is scaled back version of AB4 which was going for an AB5 at the time), or could still be valid despite the first impressions due to other factors not immediately obvious.
4. Often, a concrete implementation of an alternative is the best way to find the full implications of it (pros/cons), compared to a hypothetical argument with small code examples i.e., the proof is in the pudding.

In this particular case, I remember the Person class used to be mutable at first and there was a ReadOnlyPerson interface that was used when Person objects were passed to a client that shouldn't be updating the objects. At some point, that design was updated to make the Person class immutable instead. It's pity that the rationale for that was not documented, which also goes to show the value of good developer documentation.