Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@Value.Modifiable mutable companion type #130

Closed
pushbit opened this issue Jul 15, 2015 · 12 comments
Closed

@Value.Modifiable mutable companion type #130

pushbit opened this issue Jul 15, 2015 · 12 comments
Milestone

Comments

@pushbit
Copy link
Contributor

@pushbit pushbit commented Jul 15, 2015

This may sound silly in the context of this project, but have you considered a @Value.Mutable annotation? I'm interested in taking advantage of the code generation for mutable data objects as well. I'm aware of the copy methods, but I would have many rapid "mutations" to make to the object and creating a new object for each one would be ill-advised on Android, where we must be sensitive to GC pressure.

@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Jul 15, 2015

Thank you for the feature request!
Prior to version 1.0 there was experimental annotation and processor @Value.Modifiable. It was removed as it was underdeveloped. General problems was with the design and semantic of modifiable instances, it's not clear what to do in lot of cases: non-nullable unset attribute was queried, what to do? throw exception, return null? what to do with default or derived attribute, when to recompute them if initializer is dependent on other attributes either set or unset? Moreover mutable instances and equals/hashCode are mutually exclusive in our view, i.e. nonsensical most of the time (despite being a common [bad] practice).
But given that need for mutability is overall legitimate. 2 approaches we're considering currently:

  • A) Resurrect @Value.Modifiable annotation to generated companion mutable class. It will have limited functionality as some features of immutable objects will have awkward/quirky semantic if applied to mutable.
  • B) Make special style to generated builder that also implements abstract value type. Essentially this will make builder an mutable implementation, with added accessor query methods.

As you can see, both options are about having mutable object being generated as companion to immutable counterpart and not a standalone. It also possible to consider more or less standalone mutable implementation, but feature set is unclear now.

General question: What features are needed for the mutable objects to be supported?
Like fluent initialization? equals? toString etc.

@elucash elucash added the enhancement label Jul 15, 2015
@pushbit

This comment has been minimized.

Copy link
Contributor Author

@pushbit pushbit commented Jul 16, 2015

Perhaps attributes that are not Nullable and don't have a Default should be required in the constructor? If the object is mutable, it probably doesn't make sense to cache the return value of default/derived attributes. Or, if necessary, the class could have some kind of dirty flag per attribute that is flipped when the attribute is set and after that the default/derived values would be recomputed. I would hope that users understand that when an object is mutated, any decisions made from equals/hashCode would need to be re-evaluated. Perhaps this could be highlighted in the documentation, if deemed worthy.

B) I wonder if a builder for a mutable class may be redundant, since the class itself can do the building. e.g. new Foo().bar(5).baz("qux") or with setter style new Foo().setBar(5).setBaz("qux"). Were you thinking that the implementation would always have setX/getX methods and the builder would just add x methods for brevity?

I would like to see the option of a standalone mutable class. I'm sure there are situations where the immutable version is unneeded, and it can always be added later if/when it is useful.

I think that covers everything that is needed (that I can think of right now). This includes fluent initialisation (e.g. new Foo().bar(5).baz("qux")) and equals, hashCode, and toString.

@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Jul 17, 2015

Good points!
Make things required in constructor will make mutable models less usable (many parameters constructors etc). I'm thinking of throwing and exception and having a method to check if it's set
Dirty tracking in a good idea, but probably too complicated for a library without strong focus on mutable models (but who knows...), skip caching, as you suggested is probably a better idea.
Making standalone solution (i.e. non-necessarily paired with immutable object) is worth considering. As of set/get methods, there are already infrastructure to detect and apply naming patterns, so there less of uncertainty. Give us some time to design/implement the solution, it may be released in upcoming versions 2.1.x.
Any other thought and suggestions welcome!

@elucash elucash added this to the 2.1 milestone Jul 17, 2015
@pushbit

This comment has been minimized.

Copy link
Contributor Author

@pushbit pushbit commented Jul 17, 2015

Sounds good, looking forward to it. I agree that a constructor with many parameters would not be very nice.

@pushbit

This comment has been minimized.

Copy link
Contributor Author

@pushbit pushbit commented Jul 19, 2015

What do you think about generating a method that would set @Default attributes to their default value, clear any arrays or Collections, and set any other @Nullable attributes to null? My intent is for something like a clear method, though I'm not sure what to do with non-nullable attributes that don't have a default value. I guess they would remain unchanged.

@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Jul 20, 2015

Interesting idea! But I would not rush to specify special ad-hoc methods for any other use case you may get, rather I'll try to explore what we could achieve with other, more general methods or combination of them. The state you are looking for may be constructed if you just construct new instance and not set any attribute.

ModifiableItem item = ModifiableItem.create();
// have state almost as you described, except that
// default attribute will be not be stored, but returned on demand
// and mandatory with throw exception if queried

Then I'm expecting we will implement method from method, analogous to builder's one. from method adjust the state of the instance with values of another instance, (not considering and uninitialized attributes, probably). This method could have variations like resetFrom vs mergeFrom. Where resetFrom may be also called copy as well

private static final ModifiableItem DEFAULT_STATE = ModifiableItem.create();
...
ModifiableItem existingItem = ...;
existingItem.copy(DEFAULT_STATE); // something like this

Having explored some of those superfluous options, I'm just curious, maybe your use-case may be justified just by creating fresh instance instead of reseting an old one? Performance-wise, oftentimes, it may be more efficient create a new instances instead of clearing existing ones.

@pushbit

This comment has been minimized.

Copy link
Contributor Author

@pushbit pushbit commented Jul 20, 2015

Copying all of the attributes from a default instance to a target instance sounds like a reasonable solution. I hope that this would also unset any @Default attributes in the target instance that haven't been initialised in the default instance, so that after the copy both instances would be externally identical.

In a standard JRE, creating fresh instances would probably be fine. But with a perspective towards Android, it's likely preferable to limit allocations and garbage collection.

@mikepenz

This comment has been minimized.

Copy link

@mikepenz mikepenz commented Jul 28, 2015

I would also love to see something into this direction. I know this project is about Immutable objects, but having the same function for Mutable object would be a huge win.

I was able to remove a lot of boilerplate code for my Immutable objects via using the annotations and would love to be able to do the same for Mutable objects. It has many advantages. In many cases i need just a simple pojo, with all the getter and setters, (perhaps with chained setters (like in the builder)) and with json support.

Beside this. thanks for the awesome lib. it really saves a lot of time.

@pushbit

This comment has been minimized.

Copy link
Contributor Author

@pushbit pushbit commented Oct 21, 2015

I'm using the 2.1.0 snapshot and so far @Modifiable seems to be working well. Just one small thing I noticed in the generated Javadoc, it refers to an Immutable* class even when I'm not generating one. For example, I think it would be preferable in the class description to say, "Modifiable implementation of [insert abstract class name instead of Immutable*]". And the @see should probably also point to the abstract class. This would also apply to the generated method docs, like for create() and toString(), which are currently referencing an Immutable* class.

@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Oct 21, 2015

Thank you for the early feedback, it helps to nail down the details! Fixes landed 4a9e8e1

@elucash elucash changed the title @Value.Mutable @Value.Modifiable modifiable companion type Oct 23, 2015
@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Oct 23, 2015

See @Value.Modifiable annotation javadoc for the details. To be mentioned in documentation

@elucash elucash changed the title @Value.Modifiable modifiable companion type @Value.Modifiable mutable companion type Oct 24, 2015
@elucash

This comment has been minimized.

Copy link
Member

@elucash elucash commented Oct 24, 2015

Released in 2.1.0

@elucash elucash closed this Oct 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.