Home

Massive outline

How to review a pull request

vhl/docs/code_quality/how_to_review_a_pull_request.md Leaving out syntactical nitpicking included in this doc for now.

Controllers

Views

Models

Testing

Unit tests

• There should be a describe block for each method
• Use a new describe block for any validations or scopes that are tested
• Complex or multiple it statements should go in a context block (a single, short it can stand alone)
• Context blocks should almost always begin with one of three words "when", "with" or "given"
• For both context statements and it statements, make sure you can easily identify the purpose of the spec, and that it describes the behavior desired, and not simply the code being tested
• When testing a method with a conditional, the test should exercise the conditional

Integration tests

• Each Scenario has its own setup and teardown, so it's acceptable to test multiple user actions that follow one another within a scenario
• If a Scenario is too long or complicated, then break it up
• Use Given, When, and Then meaningfully
• Do not use any instance variables except @user (for the current logged in user)
• Use "([^"]*)" in the body to allow specifying things like Program titles, Course names, etc.

UI

Presenters

Modules

Refactoring Guidelines doc

vhl/docs/code_quality/refactoring_guidelines.docx (these links suck)

Controllers

• Keep methods to < 6 LOC, for reals. (You can't just hide the other lines in a private.)
• No business logic

Views

• Keep presentation separate from data
• Use a Presenter for complex logic
• Use Helpers only for presentation (not logic)
• No data structure manipulation (looping is ok)
• No variable assignment

Models

• Keep methods to < 6 LOC
• Leverage built-in ActiveRecord capabilities properly
• Using named_scope for composable queries
• Single Responsibility Principle
• Law of Demeter

Specs

• Keep before blocks to < 8-10 LOC on unit tests. Integration setup may be longer.

UI

See Mike's new doc.

ActiveRecord Model Organization

Inclusion order:

1. modules should be included first, so we know what behavior we are adding to the class.
2. default_scopes
3. associations (in this order):
4. has_many
5. has_one
6. has_and_belongs_to_many
7. belongs_to
8. named scopes
9. validations
10. callbacks
11. class-level macros (delegate, accepts_nested_attributes, etc)

General Coding Principles

1. DRYness
2. Refactor cycles (vs just moving things around)
3. Repo management strategy
4. cyclomatic complexity

PR Review

Modules: when and how

Enumerables

More than one Enumerable method in a method ramps up complexity exponentially. Break these down into smaller methods. Then, make sure you’re using the correct Enumerable method. If you are aggregating results into an array or hash, use inject; don’t use an external variable and an each loop. If you need to transform elements, use map/collect. Use count instead of select { block }.size. Use any? rather than select { block }.size > 0. Use none? Instead of select { block }.size == 0. Since we use Enumerable every day, we should use it properly. There are analogues for some of these methods in AR that generate SQL statements; these can be very helpful as well!

Maintain the same level of abstraction in a method. That is, don’t mix high- and low-level calls in the same context. This is good:

def some_method
  something_valid? && things_have_some_value?
end

This is bad:

def some_method
  something_valid && things.any? { |thing| thing == @some_instance_var || thing > @some_other_var }
end

That last enumerable should be moved into its own method, tested, and the method it was extracted from can now be composed of smaller methods with meaningful names.