lisa/reve — Issue #4: Split up classes
Author: radar
Created: 2011-11-23 06:57:04+00:00
State: open

Hi,

Someone called ech0\` came into the #rubyonrails channel and asked a question that was regarding your library. In trying to help them debug their issue I came across the `classes.rb` file in your library and was... shocked. For three reasons.

One, and this my favourite thing: You've *documented* things. You have spent some of your time documenting things! Most people *don't* do this and it grinds my gears. Thank you thank you thank you for doing this. Awesome work!

Secondly: There is *so much code*. You had to have spent a lot of time writing and testing all of this. That's another thing: TESTS. There are *actual **real** tests* for this. Amazing.

Thirdy, and more seriously...

Throwing all the classes in the one file is a developmental nightmare in Ruby, for a number of reasons. The first of course being that finding *anything* in that file becomes hard. Like, I tried finding the Character class (it's [here](https://github.com/lisa/reve/blob/master/lib/reve/classes.rb#L584-592) btw) to help our friend, thinking it would be somewhere else.

That somewhere else would be `lib/reve/character.rb`, and that file would define *just* the `Reve::Character` class. A grand total of 15 lines of code.

By doing this, any developer working on this project, or anybody wanting to know how exactly this thing *works* would be able to go into their Editor of Choice (read: Vim, obviously) and then find the `lib/reve/character.rb` file to find out how exactly this `Reve::Character` class *works*.

Added bonus! Things like [this class accessor](https://github.com/lisa/reve/blob/master/lib/reve.rb#L60) would go into this class. Like this:

```ruby
module Reve
class Character
def self.url
"http://api.eve-online.com/account/Characters.xml.aspx"
end
end
end
```

You could go one step further. See that first part of the link? `http://api.eve-online.com/`. You could move that into the Reve module and reference that everywhere you need to, rather than having it in a million (ok, that's hyperbole, but you get the point!) different places.

Finally, this would also allow you to do things like this in your library, which I think is the coolest part:

**lib/reve.rb**
```ruby
module Reve
autoload :Character, "reve/character"
...
end
```

While this *seems* innocuous, what it actually allows you to do is really cool. People only need to `require 'reve'` in their libraries / applications. When they attempt to reference `Reve::Character` for the *first* time, it will autoload the file specified, basically doing this:

```ruby
require 'reve/character'
```

Next time, it's there and loaded and so it won't be re-required. The benefit of this is that it will only load the classes when needed to, meaning that Reve won't need to load 1000+ lines of code to begin with and will only load the things it needs to load when it needs to load them.

So.

What you can do is this:

1. Investigate how you can split the classes into separate files. `lib/[gem]/[class].rb` is the format that all the cool kids are doing. I think we should stay with the cool kids, primarily because they're cool.
2. Move anything to do with a class into the class itself. Things like [this](https://github.com/lisa/reve/blob/master/lib/reve.rb#L638-643) where you're retrieving a list of characters would be better off inside the `Character` class. That way, people could do `Character.all` or something like that.
3. Split your tests out as well. Currently it's all in one huge file. This just leads to better organisation. To add a new feature to the `Character` class for example, I would have two buffers open in Vim: one for the Character class itself and one for the test. Then I would do my changes. As it is right now, it's very painful to do this.

**I believe in your abilities enough to fix this**. From what I've seen, you've not written *horrible* code, it's just *unorganised* and that's really easily fixed.

Please don't take this as a criticism of your code, it's not. It's just some friendly advice from another Ruby programmer who obsesses probably a bit too much over the organisation of everything. I'm just trying to help.

Congratulations if you've read this far, and good luck!