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!

Comment From: wilkinsona

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.

Comment From: woosung1223

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.

Comment From: wilkinsona

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.

Comment From: woosung1223

@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.

Comment From: philwebb

Thanks very much @woosung1223!