-
Notifications
You must be signed in to change notification settings - Fork 1
Minor auto-refactor code cleanup on a massive scale #248
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
| { | ||
| row = Table.insert(u, ti, row); | ||
| templateId = (Integer)row.get("rowid"); | ||
| row.get("rowid"); |
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.
That's interesting how it did the refactor here. Assigning a value to templateId is unnecessary, which it found. However, leaving line 139 as-is should not happen. Line 139 can be removed.
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.
@labkey-jeckels : do you want me to make this edit here, or will you do these on your machine?
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.
That's interesting how it did the refactor here. Assigning a value to templateId is unnecessary, which it found. However, leaving line 139 as-is should not happen. Line 139 can be removed.
It doesn't modify the right-hand side of the operation since it doesn't know if there are side-effects (in general).
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 removed the line completely. I agree with Nick's assessment of why the refactor made the edit it did. I've seen cases where it completely removed method calls that were part of unused assignment statements, but only in cases where it knew there was no side effect, like a trivial get method.
bbimber
left a comment
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.
please see comment about AssayHelper line 139
Rationale
We can cleanup tens of thousands of warnings across our codebase with IntelliJ's autorefactors. In some cases, this adopts newer, leaner syntax. In others, it simply removes code that's not serving any purpose.
Related Pull Requests
Changes