-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Refactor textRows.readRow to improve readability #1123
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
Conversation
Refactor textRows.readRow to improve readability: * simplified flow: * reduced maximum indent level * return error early * remove 3 'continue' * remove one type cast * remove one obsolete comment about readLengthEncodedString * -6 lines of code
19a00e3 to
d4d83cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't err ignored now?
Seems like this should also have a test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I just saw the comment above.
This is a semantic change, which I disagree with. We should not introduce any silent fallback behavior. The user requested a parsed time and should receive an error, not something unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it looks like I misunderstood the original code. And this shows that it must be refactored to make it easier to follow 😉 .
I will fix my code once the test suite is extended to cover that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still must be changed before I can approve this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer one more indent (e.g. put switch statement inside if mc.parseTime {, instead of duplicated dest[i] = b.
But this is matter of taste. The current PR code is OK too.
Co-authored-by: Inada Naoki <songofacandy@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still must be changed before I can approve this change.
Uh oh!
There was an error while loading. Please reload this page.
Description
Refactor textRows.readRow to improve readability:
Checklist