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

Refactor NumberConversionUtil and toString() of CookieList & XML Classes. #831

Merged
merged 5 commits into from
Dec 3, 2023

Conversation

adityap27
Copy link
Contributor

Hello @stleary,

I have made 3 refactorings.

1. Renamed Variable boolean b in toString() method of CookieList Class.
boolean isEndOfPair is more meaningful as compared to just boolean b.

2. Introduced indentationSuffix variable in toString() method of XML Class.
There are multiple long expressions which are using (indentFactor > 0) ? "\n" : "" sub-expression. indentationSuffix is introduced to replace this sub-expression to improve readability and reduce duplication.

3. Added method isNumericChar(...) in NumberConversionUtil Class.
There are some digit-related conditions which may look complex to some developers due to multiple conditional operators.
Eg: if ((initial >= '0' && initial <= '9') || initial == '-' )
and if(at1 == '0' && at2 >= '0' && at2 <= '9')

The sub-condition c >= '0' && c <= '9' is moved to a method isNumericChar(...) to reduce the complexity of these conditions and better readability.

Notes:
All the unit tests are passing successfully.
image

This code is compatible with Java 6.

@adityap27
Copy link
Contributor Author

Hello @stleary and @johnjaylward,

Can this be reviewed and merged please?

@adityap27
Copy link
Contributor Author

Thanks for starting the review process @stleary.

I would like to ask if there is any possibility of merging this before the 26 Nov day end?
Actually, this is a part of my university course assignment, where I would be getting a few bonus points if this gets merged on time. Sorry, if this gentle request sounds casual. I really understand and respect your time.

I am happy to modify the changes as per your feedback.

@stleary
Copy link
Owner

stleary commented Nov 27, 2023

@adityap27 Out of sync, please merge master to your branch.

@adityap27
Copy link
Contributor Author

@stleary - done, now in sync.

@stleary
Copy link
Owner

stleary commented Nov 27, 2023

What problem does this code solve?
Refactoring to improve code readability

Does the code still compile with Java6?
Yes

Risks
Low

Changes to the API?
No

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
Yes, see first prompt

Review status
APPROVED

Starting 3-day comment window

@stleary stleary merged commit d452169 into stleary:master Dec 3, 2023
7 checks passed
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

Successfully merging this pull request may close these issues.

3 participants