kamikat / moshi-jsonapi

JSON API v1.0 Specification in Moshi.
MIT License
156 stars 34 forks source link

Getter/setter or public fields #21

Closed kamikat closed 8 years ago

kamikat commented 8 years ago

Hi, @mreichelt I'm trying to merge #16 but found the Error class uses a so called getter/setter pattern. This should not be an issue about the PR but the whole project. So here's another issue.

In my idea, although getter/setter is considered better in encapsulation, we just do not need polymorphism/method overriding on either Document or Resource. And using a getter/setter for every property seems a little verbose. Could you please make a short explanation about that? So that we can decide whether to update any other classes in this project.

mreichelt commented 8 years ago

TL;DR: I would go with private fields and setters+getters.

Actually, in certain situations it is ok to have public fields. But the problem with public fields in Java (in contrast to other languages like C#) is that once public fields are used everywhere, you have no chance of reacting to changes of the field from outer classes. I found a reasonably good explanation on StackOverflow: http://stackoverflow.com/questions/7959129/whats-the-deal-with-javas-public-fields/7959208#7959208

It's not always a sin to leave a field public, but you limit yourself severely in that you're coupling the implementation to the classes using it. Say later you want to add a listener that notifies you anytime the value in X is set. You have no way to do this without refactoring everything. If you implement setX() and hide the field itself, you will only have to change the implementation of setX() to notify you that a change was made with no change to classes utilizing this method.

My personal opinion is to hide all the fields behind setters/getters. Especially the Resourceclass - which our models have to extend from - exposes it's _id, _type and _doc variables to every user of the model. We have a code style analyzer called 'checkstyle' in place which gives a warning for public fields.

mreichelt commented 8 years ago

Additional note: I would go forward with the setters/getters for the new JsonApiError class, but I would preserve the change of _id, _type and _doc for a major version update – because hiding those is a big API change.

kamikat commented 8 years ago

I've under evaluated the complexity of a model hierarchy. Accessor support is taken into consideration and should be released in next minor version change.

Public field access will be deprecated to keep compatibility and be removed on next major version change.

Thanks for your advice.