Closed rye closed 9 years ago
This is fine. We can also remove depending more than is necessary, as is in the case of TeamsController
requiring frecon/models
.
I do not agree with MatchNumber
and Position
being boilerplate code. I do agree that putting model.rb
if it still existed and controller.rb
into the base
directory would be a good idea.
I do not agree with requiring specific parts of the gem in bin/frecon
. I think that just requiring the gem will do.
On a somewhat-unrelated note, we should really expand the file db.rb
to database.rb
.
For TeamsController
, we do need to require frecon/models/team
(or whatever it is) from within that, but we don't need to require all of the models.
MatchNumber
and Position
are really wrappers for in-line string parsing, moving what would normally be a large amount of non-DRY-compliant code into an object that can be used to represent their respective things. Whatever words you use to describe them are your preference, but we can keep them where they are. model.rb
could have a lot more code, I just added it to DRY up the model code a bit.
I'm hella not looking at the actual code when I make issue comments, since I'm, you know, on vacation and I'm stuck with my craptop until Kernel 3.17 comes out. Obviously, requiring the entire gem is much better than one specific part. My mistake. If we're already requiring the entire Gem, then nothing needs to change.
Yes, filenames and shortcut variable names like db
should definitely be changed to their corresponding expanded forms. There's no shortage of storage space on any of our hard disks and making the code itself more descriptive would be very good. Also, all of our code should be thoroughly documented, and our custom/non-standard classes (Position
, MatchNumber
, and the Model
and Controller
classes) should have large amounts of explanation as to their purpose. The changes suggested in this paragraph will help to improve our code climate so that others can come in and fully understand what's going on. I'd say it'd be best if a non-Rubyist would be able to know what's going on from the comments and the general noob-friendliness of Ruby.
Since converting Issues into Pull Requests is deprecated, I'm going to close this issue and open a PR with the basic gist (git it?) of this issue, to start working on this. My opinion has changed; see that PR for information regarding it.
One of the compatibility problems we might have with older/different Ruby implementations is with dependencies of library files. Ruby's
require
returnstrue
if the file is newly-loaded andfalse
if not, so I think it'd be a good idea to map out dependencies for each of the files and then make it such that each of the files requires what it needs.For example, if I have these files:
on some Ruby implementations it might work to run
ruby a.rb
because the interpreter loadsb.rb
beforec.rb
, but on some implementations (I think we've had similar problems with dependency on"json"
on @fishyfish235's side) it wouldn't because those might be loaded differently (perhaps multithreaded, for example).I suggest that we resolve this by determining everything that each file depends on and explicitly placing
require
statements in all of the.rb
files in thelib/
directory. I do not mean that we should require every single thing in thebin/frecon
file, but only files from which code is used (i.e.bin/frecon
would requirefrecon/server
to run the server)I'd also suggest that we create a
base
directory for housing boilerplate code, such as thelib/model.rb
andlib/controller.rb
files, as well aslib/match_number.rb
andlib/position.rb
. We can also put modifications to Ruby classes in those files.The main thing is that we need to standardize what we have so that we aren't expecting lame shortcuts to work. Thoughts?