Skip to content

Should connect require connect or board #854

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

Closed
iHiD opened this issue Aug 30, 2018 · 13 comments
Closed

Should connect require connect or board #854

iHiD opened this issue Aug 30, 2018 · 13 comments
Labels

Comments

@iHiD
Copy link
Member

iHiD commented Aug 30, 2018

Someone emailed me about this.


Take a look at these two lines:
https://github.com/exercism/ruby/blob/master/exercises/connect/connect_test.rb#L2
https://github.com/exercism/ruby/blob/master/exercises/connect/connect_test.rb#L15

require_relative 'connect'
...
Board.new(board)

We require a file called 'connect' but then require a class called Board. Seems a little un-Ruby-like. Should we require board instead? Thoughts?

@Insti
Copy link
Contributor

Insti commented Aug 31, 2018

Or change the class name to connect. Connect.new(board)

@Insti
Copy link
Contributor

Insti commented Aug 31, 2018

Thoughts:

I agree that to match Ruby conventions the require and the class should have the same name. This is useful for organising code in large projects, but is less of an issue when there is only one file in a project.

The Exercism ruby track convention is for the required file match the slug.

Are there any other exercises that exhibit this mismatch? - yes many.

As a solver I'd rather have the require be the name of the slug so I can find my solution to a particular exercise by slug.

Imagine if there were chess or go or reversi exercises that all tested 'Board' classes.

The student can change the name of the required file in their tests if they want.

Or create the connect.rb file that contains:

require_relative `lib/board`

In Rails application.rb generally does not contain a class/module called Application (edited)

Potential disclaimer: I'm conflicted by 'ease of maintaining the track' since require_relative "#{slug}" is much easier to implement in generators.

Conclusion: I think I'm OK with the current situation. (But am open to having my mind changed.)

@Insti Insti added the question label Aug 31, 2018
@petertseng
Copy link
Member

If it is simultaneously important to have the filename match the exercise (connect) and also have the class name be meaningful (and assuming a Board carries more meaning than a Connect), one suggestion I might think of is a module named Connect (in file connect.rb), and the module contains a class Board

@Insti
Copy link
Contributor

Insti commented Aug 31, 2018

That is a good solution. It would become:

require_relative 'connect'
...
Connect::Board.new(board)

(I'm still not sure it's important though.)

@sfairchild
Copy link

These are just my opinions. it is important to follow best practices no matter how simple the challenge is. There are time where breaking convention makes sense and is even needed sometimes, but when you are teaching someone new it is best to stick with the best practices unless you explain why you deviated.

require_relative 'connect'
...
Connect::Board.new(board)

This make sense

@kotp
Copy link
Member

kotp commented Sep 1, 2018

Robot name is one other one that I can think of where the class and the filename differ.

@Insti
Copy link
Contributor

Insti commented Sep 1, 2018

There's a bunch of them that I've seen by eyeballing it but not worked out the pipeline chain to make a nice list. It starts with egrep -r 'require|\.new' exercises/ (Update: but that misses class methods)

@Insti
Copy link
Contributor

Insti commented Sep 2, 2018

require_relative 'connect'
...
Connect::Board.new(board)

Is it worthwhile inventing a redundant class/module just so we can do this?
Surely it's better just to have the class named 'incorrectly' for the file it's in.

@Insti
Copy link
Contributor

Insti commented Sep 2, 2018

While we're talking about conventions:

The test file is called connect_test.rb
The file it is testing should be called connect.rb

It's important[citation needed] that the test file match the exercise slug.
Therefore it is important that the solution file should be slug.rb

@iHiD
Copy link
Member Author

iHiD commented Sep 3, 2018

(cc @kytrinyx)

@kytrinyx
Copy link
Member

kytrinyx commented Sep 4, 2018

I don't know that it's important that the test file match the exercise slug, but I think that this is the convention in the Ruby track, so I'm all for being consistent here 👍

@emcoding
Copy link
Contributor

Do we need to take action on this?

@kotp
Copy link
Member

kotp commented Dec 15, 2018

I don't think so, but I think it is good that the discussion is here. Closing (feel free to reopen though.)

@kotp kotp closed this as completed Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants