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

hello-world: Modify tests to reject name == "" comparison #280

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

otraczyk
Copy link
Contributor

@otraczyk otraczyk commented Feb 7, 2017

Hello World was not supposed to be tricky, but I think I've found an uncovered case.

In some solutions people were using name == "" to check for empty string, as opposed to name.equals(""). The == operator in java compares object references, not values. Currently the tests pass in such case, but it's only a coincidence, since they compare to the same instance of the String constant "" in JVM's constant pool. For newcomers, it could be a dangerous misconception. This change avoids it.

It's my first PR, so any feedback and guidance is welcome.

I've also considered making a separate test case instead of changing current one, to clarify the problem. Do you think that would be better?

@jtigger
Copy link
Contributor

jtigger commented Feb 8, 2017

Hello @otraczyk!

Thank you for taking the time to make this improvement. These are exactly the kinds of refinements — ways to improve the learning experience — we just love.

In terms of making this a separate test... yeah, I think you're onto something there. We wouldn't want to leave someone stuck at "wow, why does == no longer work?!".

I was thinking of suggesting a comment, but a separate test is starting to feel more appropriate to me too. You can bake into the name of the test the lesson being conveyed.

Please feel free to make that change and we'll review.

Cheers!

@jtigger jtigger self-assigned this Feb 8, 2017
@otraczyk
Copy link
Contributor Author

otraczyk commented Feb 9, 2017

Hi @jtigger . Thanks for your attention!
I've added a separate test. Do you think the case is clear enough?

@jtigger
Copy link
Contributor

jtigger commented Feb 9, 2017

I do! Thank you for making the adjustment, Olga.

Congratulations, you've successfully completed your first pull request!

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.

2 participants