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

Consider making value classes final #8

Closed
joshdifabio opened this issue Oct 27, 2018 · 6 comments
Closed

Consider making value classes final #8

joshdifabio opened this issue Oct 27, 2018 · 6 comments
Milestone

Comments

@joshdifabio
Copy link

The value classes should really be final.

@BenMorel
Copy link
Member

Good point, I usually make my classes final unless they are explicitly meant to be extended, not sure why I did not do this at the time.

@BenMorel
Copy link
Member

Done in 99c1f65, will be released as 0.2.0 once #10 is complete.

@BenMorel BenMorel reopened this Mar 8, 2019
@BenMorel BenMorel added this to the 0.2 milestone Mar 8, 2019
@natebrunette
Copy link

I was going to suggest adding better support for extending classes like ZonedDateTime. I'd like to be able to extend it, and it would be helpful if static was used instead of the class name. I want to be able to augment the class with app-specific methods, but currently can't without copying most of it.

@joshdifabio
Copy link
Author

These are value objects -- you shouldn't really be extending them.

Testing equality is fundamental on VOs and being able to subclass a VO complicates or breaks that feature.

@BenMorel
Copy link
Member

I'm on the same page as @joshdifabio here; you should favour composition over inheritance, i.e. create your class from scratch, wrapping an instance of ZonedDateTime and delegating parts of the work to it. Yes, some methods will just be proxies to the underlying object methods, but you'll expose only the methods you need, and are free to change their behaviour as you wish (like making them return an instance of your class).

I'm usually against extending classes from a library, unless they've been specifically designed for this purpose. In addition to not being good practice, extendable classes add to the maintenance burden by adding backwards compatibility promises to internal code.

Maybe if you tell us a bit more about your use case, we can help you refactor it towards composition. What are you extending ZonedDateTime() for?

@BenMorel
Copy link
Member

BenMorel commented Jan 8, 2020

Done in 9625e84 and released in 0.2.0.

@BenMorel BenMorel closed this as completed Jan 8, 2020
@tigitz tigitz mentioned this issue Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants