Code Organization

[latortuga]

Someone got downvoted pretty hard on reddit for saying that your code organization needs work - that guy was right but he wasn't helpful so here's some practical tips on how you can improve your code:

• Don't pollute the lib directory. When you call require 'fancy_gem' Ruby puts all of the gem's lib directory directly on the load path. This means that you need to respect it and not put more in there than is absolutely necessary. In your case, you need to move everything out of lib and into a new 'chordy' directory inside lib (lib/chordy/). The only thing that should stay is the file that people need in order to require your gem, e.g. chordy.rb
• Don't pollute the global namespace. Right now when your gem is required, you include two modules in the global namespace - don't do this! Instead, use a private namespace named after your gem.

module Chordy
  class Chord
    ...
  end
end

This helps keep your class names specific to your namespace so anyone using a similar name in their app won't have issues with conflicts. This also means you have to go through all of your chord classes and update them to be nested inside a module.

• Good grief man, 12 classes with 19 constant-returning methods each? None of those classes deserve to be classes. You never even instantiate any objects from them! All of that data is just configuration information and should be in a yml file that is read at runtime.

c_sharp: 1,2,3,4,5,6

require 'yaml'
chord_data = YAML.load(File.open('chords.yml'))

def chord_lookup(desired_chord)
  chord_data[desired_chord].split(',')
end

• Eval is not evil but your use of it is. Eval will get you into trouble, especially the way you're using it to dynamically dispatch methods. Try Object#send instead.
• Globals, so many globals. For things that don't change, use constants that are in your namespace. For things that can change, use instance variables. This is just another instance of polluting the global namespace - be polite to people using your code.

[darth10]

@latortuga Thanks for the feedback :heart:

Could you explain a bit more on the evil eval? I mean I could use Object#send instead for eval in the play function, but what about the use of class_eval? Can you suggest a better way of adding methods dynamically?

I also intentionally kept the chords as constant-returning methods of Chord instead of part of data/config files. The reason is easy sub-classing and monkey-patching, which would be useful if you wanted to refine a chord's definition as per your needs. If there's a way I can do that with data files, I'll surely go for it. Any suggestions?

[latortuga]

@darth10 sure thing - I was only talking about the regular ol' eval, not the class_eval which definitely has uses. The biggest problem with your (previous) usage of eval is that it uses unchecked user input to dynamically dispatch methods which means user input can execute arbitrary code. I think you can probably see why this is a bad thing. As far as defining methods with class_eval, more power to you!

Couple more tidbits:

• Switch to the newer, better supported debugger gem.
• Add a gemspec and make your Gemfile point at your gemspec:

# Gemfile
gemspec

# chordy.gemspec
...
  s.add_development_dependency 'gem_name', '~> 2.3'
...

You will have to unignore the gemspec file. Take a look at some other open source projects for information about how to put one together.

[darth10]

Simply awesome input @latortuga!

I actually had added the gemspec file initially, but later removed it as I noticed you don't really need it in the repository to build a gem. Will add one soon.

I'll post these as new issues later.