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

[BUGFIX] Don't clobber $i #361

Merged
merged 1 commit into from
Feb 23, 2017
Merged

Conversation

erayd
Copy link
Contributor

@erayd erayd commented Feb 23, 2017

While using $i here follows convention, assigning to it here can occasionally clobber an existing value. It doesn't cause any testsuite errors, but is still undesirable behavior, and should not be used this way.

This PR renames the variable used within the set-defaults section to avoid having foreach() overwrite it.

@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017

It looks fine, but it's not immediately clear to me which existing value you're referring to. Something earlier in the file?

@erayd
Copy link
Contributor Author

erayd commented Feb 23, 2017

Yep. There is already a variable named $i elsewhere in validateCommonProperties(), so I shouldn't be naming my completely unrelated variable $i, as doing so means that whatever somebody else put in $i before I came along gets replaced when foreach() runs.

@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017

Ah, yes, it shadows the $i parameter. LGTM

@bighappyface bighappyface merged commit ef3ee83 into jsonrainbow:master Feb 23, 2017
@shmax
Copy link
Collaborator

shmax commented Feb 23, 2017

(come to think of it, it's pretty hard to think of a worse parameter name to be sprinkling through the code base than $i)

@erayd erayd deleted the bugfix-noclobber-i branch February 23, 2017 23:11
@erayd
Copy link
Contributor Author

erayd commented Feb 23, 2017

Indeed. It's nice and concise though... but not particularly descriptive, and conflicts with the conventional programming use of $i as a temporary loop variable.

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.

3 participants