petey5king/bbgcli — pull request #2 by jcsalterego (2011-10-30 13:44:24+00:00)
Title: Beginnings of a Refactor

## Motivation

Open source is great! I hope my feedback provides some good insight into best practices. I'm pretty confident the things I mention below follow conventions and aren't just my weird coding style.

## Warning

There might be a few bugs in the code since I don't have a BlueBox account to test with.

I didn't get to the `LB_*` classes either.

## General Guidelines Found In This Refactor

### Avoid Class Variables Unless You Really Need Singletons

So, instead of

class Foo
@@bar = 3
def self.baz
@@bar
end
end

Foo.baz # => 3

you should use

class Foo
def initialize
@bar = 3
end
def baz
@bar
end
end

f = Foo.new
f.baz # => 3

The way you were using them with the `BBGAPI` module effectively created a global, singleton state. If you had another config to load, tough luck.

Global configs are actually one of the more debatable use cases for singletons, but given this particular refactor, I thought it made sense to tie a `Config` to a `Client` instance.

## Avoid Inlining Code

The original loading of the `@@config` variable in `BBGAPI` had an inlined `begin...rescue`, which you pretty much never want to do. You want to wrap it in a method, and then explicitly call the method. This was probably not as obvious since your class was written with static class methods, so there was no constructor.

This also makes testing easier, but practically speaking, if you wanted to reload the config, you would be able to call the method.

## One File Per Class

The ruby convention is putting a file per class, and including files found in the subdirectories. Example:

blahface.rb
blahface/foo.rb
blahface/bar.rb
blahface/baz.rb

And in `blahface.rb`:

require 'blahface/foo'
require 'blahface/bar'
require 'blahface/baz'

## Use the Or-assignment Idiom (Memoization)

One common idiom in Ruby looks like the following:

class Foo
def bar
@bar ||= 20
end
end

Foo.new.bar # => 20

Basically, any instance variable you attempt to conjure up defaults to `nil`. What the or-assignment operator does is effectively `@bar = @bar || 20` -- if `@bar` is `nil`, assign `@bar` to 20. If `@bar` is non-nil, well, assign it to itself (no change).

This technique goes hand-in-hand with *memoization* (not a typo), which roughly means, don't recalculate something if you can just save the value.

That's exactly what I do in the config loading code:

def config
@config ||= Config.new(@config_file)
end

## Use Constants Where Appropriate

For variables that won't changed or reassigned in the code, use constants (denoted in all-capital). For example, `API_URL` is a suitable replacement for `@@api_url`. Define it at the top of the class or module.

Speaking of which, the following conventions exist in ruby:

* `foo` or `foo4` or `baz_qux` - variables are lowercase
* `SomeClass` or `Physics` - classes are camel-case
* `LOUD_NOISES` - constants are uppercase

Thus, `BBGAPI` should actually be a constant, but since it's defined as `module BBGAPI` or used as `BBGAPI::`, the code works. However, it's an important convention to keep in mind.

## Don't Use Exceptions for Flow Control

This just means don't use `begin...rescue` when you can use conditionals. For example, the original code tried loading the config file, and if that failed (for whatever reason: the file was missing, the user hit ctrl-c, the sun exploded), it would try to create one interactively.

A better approach is to do a few checks such as `File.exists?(@config_path)`, before attempting to open it.

This is perhaps not the best example after all, but just watch out for abusing `begin...rescue`.

## Properly Abstract

Ruby is an object-oriented language, among other things, so use it to your advantage.

The Config object is brand new and a perfect example of taking advantage of OO. Originally, the code tries to load the YAML config into a hash, but having an instance of Config, created by just passing a config_path, means you can do things like `#load`, access it via `#[]` as you would as hash, and `#save` at will.

## Document or Reconsider Assumptions

You should probably document somewhere that an SSH key is used, and in particular an id_rsa instead of an id_dsa. Another (undocumented) assumption is that the file is `./bluebox.yaml` -- which I've preserved -- but note that it's a relative path, which of course relies on where the user is located.

## Use Modules for Namespacing

Unless you're writing a Mixin, modules are most likely going to be used for namespacing. If you're writing a method directly in a module, you probably mean it for a class within the module.

On a similar note, if you see that you're using a particular prefix often, it might be an indicator you need a submodule. More specifically I see that for the `LB_` prefix, e.g. `BBGAPI::LB_Applications` would become `BBGAPI::LB::Applications`.

# Hope that helps!

You know where to find me if you have questions.