Skip to content
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

Fix variable hides variable #2111

Closed
wants to merge 1 commit into from
Closed

Fix variable hides variable #2111

wants to merge 1 commit into from

Conversation

GregoryLundberg
Copy link
Contributor

Introduced by 49d0af3

@AI0867
Copy link
Member

AI0867 commented Oct 20, 2017

I would prefer wrapping the previous team* t and associated if-block in another block so it goes out of scope before the for loop.

Alternatively, you could attempt to turn that check (does this side exist, and if so, does it own this village) into a oneliner, though I'm not sure if that would remain very readable.

@GregoryLundberg
Copy link
Contributor Author

I, too, would prefer to put it into a block of its own. But that presumes the future reader (1) notices the extra two characters, and (2) realizes what they do. A more natural read would assume the same variable in use in both places; which is semantically true but syntactically false.

This is a prime example of code which should be rewritten to eliminate the smells.

But as a patch, simply renaming one of the variables is more clear. So, I went with a clear, understandable, commit history, instead of rewriting perfectly good code just to eliminate a bad smell.

Vultraz added a commit that referenced this pull request Oct 21, 2017
…inted out

Also fixes the issue brought up in #2111.
@Vultraz
Copy link
Member

Vultraz commented Oct 21, 2017

I've pushed an alternative fix: fcaa4ef

@Vultraz Vultraz closed this Oct 21, 2017
@Vultraz
Copy link
Member

Vultraz commented Oct 21, 2017

@GregoryLundberg BTW, please feel free to introduce fixes such as the one above in future. It wasn't too intrusive and made the code neater overall.

@GregoryLundberg GregoryLundberg deleted the GL_Fix_49d0af34 branch October 21, 2017 14:28
GregoryLundberg pushed a commit that referenced this pull request Nov 30, 2017
…inted out

Also fixes the issue brought up in #2111.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants