Skip to content

Conversation

woosung1223
Copy link
Contributor

I just refactored tokenize method in BasicJsonParser.

The modifications are as follows.

  • merged inObject and inList to inData. Since we are only interested when both values ​​are 0, we can merge them into one variable. If there is a better variable name, I'd appreciate suggestions.
  • changed to use the enhanced for statement.
  • changed to use enhanced switch statement.
  • for readability, changed inValue to inQuote.

There were no problems when I tested it myself, but if there are any errors, I welcome your comments.

Thank you!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 1, 2024
@wilkinsona
Copy link
Member

Thanks for the proposal.

Unfortunately, I don't think we should make this change as it currently is. Like any change, it brings with it the risk of regression and I don't find the changes to have enough benefit to justify that risk. In particular, combining inObject and inList means the parser is unable to detect invalid json like {"key":["a"} which feels like a step backwards even if it doesn't have such error detection today.

Just moving to an enhanced switch statement may still be worthwhile as it avoids some unnecessary if checks and I think does improve the readability. Would you like to update your proposal to only include that change so that we can consider it? If you do so, please note that we use tabs rather than spaces for indentation and your changes should not alter that.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Oct 1, 2024
@woosung1223
Copy link
Contributor Author

thanks for feedback!

I get it. Clearly, merging variables is a step backwards for the future.
So, you said it's okay to use enhanced-switch statements, but is it okay to use enhanced-for?
If that's okay with you, I'll take what you said into account (using tabs) and fix it.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 1, 2024
@wilkinsona
Copy link
Member

IMO, changing to a for-loop from a while-loop doesn't improve things that much. However, the move to a switch statement is beneficial as it makes it clear that current should only be evaluated once each time around the loop and also avoids processing of any subsequent ifs that won't match if one of the earlier ifs has matched.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 1, 2024
@woosung1223
Copy link
Contributor Author

woosung1223 commented Oct 2, 2024

@wilkinsona
Thanks for feedback again!

I just modified it to only use the enhanced-switch statement and I would be appreciated if you check one more time.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 2, 2024
@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Oct 2, 2024
@philwebb philwebb added this to the 3.4.x milestone Oct 2, 2024
@philwebb philwebb changed the title refactor tokenize method in BasicJsonParser Refactor tokenize method in BasicJsonParser Oct 2, 2024
@philwebb philwebb changed the title Refactor tokenize method in BasicJsonParser Refactor BasicJsonParser to use enhanced switch Oct 2, 2024
@philwebb philwebb modified the milestones: 3.4.x, 3.4.0-RC1 Oct 2, 2024
@philwebb
Copy link
Member

philwebb commented Oct 2, 2024

Thanks very much @woosung1223!

philwebb pushed a commit that referenced this pull request Oct 2, 2024
@philwebb philwebb closed this in 9ea6246 Oct 2, 2024
philwebb added a commit that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants