Skip to content

Unnecessary assignments are removed. #1107

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.

Sign up for GitHub

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

Jump to bottom
Merged
ctubbsii merged 6 commits into from
Oct 16, 2020
Merged

Unnecessary assignments are removed. #1107

ctubbsii merged 6 commits into from
Oct 16, 2020

Conversation

@kamaci
Copy link
Member

@kamaci kamaci commented Oct 16, 2020

No description provided.

ctubbsii
ctubbsii requested changes Oct 16, 2020
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @kamaci ; These look okay to me, but I made an additional suggestion.

@ctubbsii ctubbsii added this to the 2.0.0 milestone Oct 16, 2020
kamaci and others added 2 commits October 16, 2020 17:23
@kamaci @ctubbsii
Variable assignment is improved.
cf569ce 
Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>
@kamaci @ctubbsii
Variable assignment is improved.
051fee8 
Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>
@kamaci
Copy link
Member Author

kamaci commented Oct 16, 2020

I've approved the changes.

ctubbsii
ctubbsii previously approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression, prev + val + "" looks ambiguous to me. I can't tell if it is supposed to be Integer.toString(prev) + Integer.toString(val) or Integer.toString(prev + val).

This is an existing problem, and not related to your changes, but if you want to fix it, it would be a good improvement.

Your changes look fine to me, though. Thanks!

@kamaci
Copy link
Member Author

kamaci commented Oct 16, 2020

The expression, prev + val + "" looks ambiguous to me. I can't tell if it is supposed to be Integer.toString(prev) + Integer.toString(val) or Integer.toString(prev + val).

This is an existing problem, and not related to your changes, but if you want to fix it, it would be a good improvement.

Your changes look fine to me, though. Thanks!

I've used prevStr variable.

@ctubbsii ctubbsii dismissed their stale review October 16, 2020 18:09

Code was changed after review

@ctubbsii ctubbsii merged commit 7131326 into apache:main Oct 16, 2020
@ctubbsii
Copy link
Member

ctubbsii commented Oct 16, 2020

Thanks for the PR, @kamaci !

asfgit pushed a commit that referenced this pull request Oct 16, 2020
@ctubbsii
Fix formatting during build after merging #1107
9633532 
This may not be related to #1107, but found while building that PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctubbsii ctubbsii ctubbsii left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@kamaci @ctubbsii