Majlanky / couch-slacker

Spring data for CouchDB and CouchDB connector
https://github.com/Majlanky/couch-slacker
Apache License 2.0
23 stars 6 forks source link

Changed fields visibility and overwrote toString() #65

Closed sokrisztian closed 2 years ago

sokrisztian commented 2 years ago

As the DocumentBase class purpose is to inherit from it, I changed the visibility of the fields from private to protected.

I also missed the toString() method. Best practice to overwrite it, said by Joshua Bloch in Effective Java: https://www.thefinestartist.com/effective-java/10

Majlanky commented 2 years ago

Thx for contribution 🎊

You are right that if there is a basic implementation of hashCode and equals, we can have some reasonable toString. Where I am little bit sceptical is the change of visibility. Can you tell be some use case where protected is handful? I can see only harmful use cases.

BTW if you like the project, can you get a star? I am a collector 🤣

sokrisztian commented 2 years ago

Where I am little bit sceptical is the change of visibility. Can you tell be some use case where protected is handful? I can see only harmful use cases.

Because DocumentBase private fields (id and revision) has public getters and setters, then I think we cannot make more harm on these fields if we change their visibility.

But why I feel pain with these private fields? For example I have this class:

@Document("films")
class FilmDocument extends DocumentBase

In that case FilmDocument doesn't have direct access to id and revision fields, just through getters. For me this is a design issue, because if FilmDocument is a document type class, then it should be ultimate control over its own id and revision fields. For example if I write a toString to this class, then it looks like this:

@Override
public String toString() {
    return new StringJoiner(", ", FilmDocument.class.getSimpleName() + "[", "]")
            .add("id='" + super.getId() + "'")             // <--- ugly :)
            .add("revision='" + super.getRevision() + "'") // <--- ugly :)
            .add("type='" + type + "'")
            .add("title='" + title + "'")
            .add("year=" + year)
            .add("director='" + director + "'")
            .add("rating=" + rating)
            .toString();
}

What do you think?

Majlanky commented 2 years ago

I think you are completely right. I went to the same conclusion when I was thinking about it. It is so obvious than I am ashamed I asked the question. Thx for the effort to respond.