-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
# Refactoring & Improvement #121
base: main
Are you sure you want to change the base?
# Refactoring & Improvement #121
Conversation
danielnd14
commented
Apr 17, 2022
- Created adapter for unsupported operations, this approach grant the concrete classes concern only on theirs busines logic.
- Created an enum for Qname instances. Enuns are singleton by default (see QNameBucket).
- Changed classes for to use the singleton instances of Qname.
- Changed utils or helpers classes to final by default.
- Created LazySupplier in a generic approach.
- Changed the classes to use the LazySupplier and removed old Supplier implementations.
- Extracted the duplicated code in a method on StreamingCell class. (see getCellTypeFromShortHandType).
- Moved some initializations to constructor in respective classes.
- Replaced "new Integer(20)" to "Integer.valueOf(20)" in Test class.
- Created adapter for unsupported operations, this approach grant the concrete classes concern only on theirs busines logic. - Created an enum for Qname instances. Enuns are singleton by default (see QNameBucket). - Changed classes for to use the singleton instances of Qname. - Changed utils or helpers classes to final by default. - Created LazySupplier in a generic approach. - Changed the classes to use the LazySupplier and removed old Supplier implementations. - Extracted the duplicated code in a method on StreamingCell class. (see getCellTypeFromShortHandType). - Moved some initializations to constructor in respective classes. - Replaced "new Integer(20)" to "Integer.valueOf(20)" in Test class.
Thanks for your interest but I don't want this massive refactor. If you have specific changes, you can submit them in smaller chunks. Essentially, I don't want to change the class structure i ways that cause backward compatibility issues. I've only just released a 4.0.0 major release and don't want to do a 5.0.0 any time soon. |
Okay, while it's a massive refactoring it doesn't necessarily break backwards compatibility. All interface contracts are maintained. And all unit tests are fully functional. |
I've applied a couple of changes but the timing is bad - if this had come in a few days ago - prior to the 4.0.0 release, I would probably have just taken the whole PR but now I am reluctant to start moving around lots of code. Let me consider some of the refactors over the next few days. |
Okay, if you need me to solve any questions, please tag my name in the comments here. |
@pjfanning, If you prefer, I can push each change separately. |
to be honest, I'm not sure the changes are needed - yes, it is tidier - but it makes binary compatibility changes that I do not want to add - to be frank, what this project lacks is test coverage - niceties about whether classes are final or not are not so important in the end of the day |