-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Refactor BasicJsonParser to use enhanced switch #42487
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 BasicJsonParser to use enhanced switch #42487
Conversation
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 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. |
thanks for feedback! I get it. Clearly, merging variables is a step backwards for the future. |
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 |
@wilkinsona I just modified it to only use the enhanced-switch statement and I would be appreciated if you check one more time. |
Thanks very much @woosung1223! |
I just refactored
tokenize
method inBasicJsonParser
.The modifications are as follows.
inObject
andinList
toinData
. 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.inValue
toinQuote
.There were no problems when I tested it myself, but if there are any errors, I welcome your comments.
Thank you!