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

Is it necessary to create new instances on the grains exercise? #113

Closed
StevenXL opened this issue Apr 16, 2015 · 2 comments
Closed

Is it necessary to create new instances on the grains exercise? #113

StevenXL opened this issue Apr 16, 2015 · 2 comments

Comments

@StevenXL
Copy link
Contributor

The new instances created by the grains exercise are all exactly alike.

Is it a better idea to make the methods class methods (instead of instance methods), and change the tests appropriately? This would keep the exercise consistent with the other exercises by testing class methods. Or am I missing the reason for having instances that are exactly the same?

If you agree with the logic of this, I can go ahead and change the exercise.

@kotp
Copy link
Member

kotp commented Apr 16, 2015

Links to the files you are referencing would be nice :)

Grains exercise tests are definitely modifiable. And should not effect any solutions that I am aware of. The example.rb file uses a Class for no obvious (to me) reason. The tests use .new rather than .allocate which it could very well do.

The test also has a lot of duplication, where the actual object could be stored and reused.

My suggestion is go ahead and work up a pull request. It will exhibit your thoughts even more completely than a paragraph of words.

Worst thing that can happen is people talk about it and it doesn't get accepted.

@StevenXL
Copy link
Contributor Author

Thanks @kotp. I'm still new to this whole thing - forgive the lack of links.

I'll do as you suggested and work up a pull request and move the conversation there. Thanks!

gchan pushed a commit to gchan/xruby that referenced this issue Oct 18, 2016
…-JSON-files

Added test messages to hamming test json file
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

No branches or pull requests

2 participants