Ruby Style Guide - TISTATechnologies/caseflow GitHub Wiki

This document is a work in progress.

General principles

This document does not seek to lay out a rigid, all-encompassing style guide. Instead, Caseflowers build on existing best practices such as Shopify's Ruby Style Guide. This document seeks to further expand on some best practices.

In general,

  • We try to follow community norms and the style of the existing code by default, and
  • We use code quality tools like rubocop and reek to enforce those standards, but
  • We are willing to deviate from standards when necessary to produce better code.

Special circumstances

The straw that broke the camel's back

From time to time, you will make a very minor change which ends up pushing something over a limit with a tool like rubocop. For example, you might add one line to an existing method, only to find that the method was already at the maximum line length.

In general, you're encouraged to roll your sleeves up and leave the code better than you found it. You might be making a minor change and realize that fixing the new warning or error requires substantially more work, but you're the one who's in the code and has an understanding of what needs to be changed, so you're in the perfect place to fix it. (Plus, the next person is going to see your name in git blame!)

Every once in a blue moon, this may not be practical. For example, adding a small bugfix may lead you to realize that the relevant section of code needs to be entirely rearchitected. Or perhaps you're addressing a bug causing an outage. In these cases, although it's not preferred, it might be best to file a followup story describing what needs to be done. But don't just chuck it in the backlog to ignore: consider leaving a comment in the code linking to the ticket, and bubbling up to your team or the relevant feature owner that followup work is needed to keep the code maintainable.

Reek:FeatureEnvy

(These reek issues are a candidate for pulling out into another article)

We use reek for scanning for code smells. One of the issues commonly encountered is Feature Envy, in which a method accesses another class's attributes more often than that of the class it belongs to. The linked page provides a good overview.

When to listen

When encountering this smell, it's a good idea to consider whether reek has a point. For example, a nine-line method in the DecisionReviewsController had deep knowledge of how to tell whether a BusinessLine had any appeals with active request issues. While it wasn't obvious where this code did belong, it didn't feel particularly pertinent to decision reviews; it was there only because other code in the controller needed to know that.

The inappropriate intimacy smell (not reek-specific), where classes have intimate knowledge of the internals of other classes, is a related sign that the boundaries of classes have been blurred.

In these cases, it's an indicator that the code you've written might belong somewhere else, such as on the class being referenced.

Ask yourself:

  • Does this code logically belong in this class?
  • Would this code be simpler if it (or part of it) was moved to the class it references?
  • Is this method trying to do too much? Should it be broken up into smaller methods?

When and how to ignore it

Like Swiss cheese, a smell isn't always indicative of a bad thing. The FeatureEnvy smell comes up a lot on our project and is often deemed to be a false alarm. We have chosen to leave it enabled because it does sometimes catch issues, but there are also a lot of legitimate reasons to ignore it.

If you've reviewed the above and believe your code belongs exactly where you put it, you should feel empowered to override the warning by adding a tag to the method, e.g.:

  # :reek:FeatureEnvy
  def tagged_hash?(hash)
    hash[:metric] && hash[:value]
  end

There are also a large number of methods overriding this in .reek.yml. Using a tag on the method is preferred over adding it to the YAML file for clarity's sake. (mattw: This is solely personal opinion. Is this something the group agrees on?)

Reek: ControlParameter

You may occasionally run into an error like Docket#appeals is controlled by argument 'ready'.

You can read more about it here. It's raised when a parameter we pass into a method controls which code path the method takes. It's a smell that "our" code and the method we're calling are too tightly coupled, and/or that, if we're (slightly indirectly) telling it which code path to take, perhaps those two paths should be separate methods.

In the case of the example error about the argument 'ready', the method being called raises an exception if we pass ready: false. A fix in this case would be easy: raise the exception ourselves, rather than calling a method where we have all the information to know that it will fail.

In general, if you run afoul of this check, you should seriously consider whether your code can be rewritten to avoid it. It may be helpful to take a step back and look more broadly at what is happening, or to tag in a friend for a set of fresh eyes. If you and a buddy are convinced the error isn't warranted, there is prior art in .reek.yml for overriding that check for a method. (Please do not add new cases of whole-class overrides in any event, though!)

ActiveRecord Scopes

Caseflow makes use of ActiveRecord scopes for many of its database queries. They are effectively syntactic sugar for class methods.

In general:

  • Prefer implementing a scope over a one-line method for simple queries
  • Avoid regularly chaining scopes (more below)
  • Avoid using default_scope (more below)

Chained scopes

While not strictly verboten, in general chained scopes are a sign that the calling class might know more about the internals of the calling class than it needs to. As argued in this blog post, consider replacing them with something more direct.

At least in earlier Rails versions, chaining scopes affecting the same column(s) led to unexpected results, which also argues for creating more direct scopes.

Default scopes

BLUF: Stay clear away from default_scope if you ever want to refer to records excluded by the default scope.

Why is Ruby on Rails' default_scope bad?

  • default_scope is used by Organization, DecisionIssue, and RequestDecisionIssue
    • It is appropriately used by DecisionIssue and RequestDecisionIssue for soft-deleting records.
    • However problems occur for Organization. Tasks can refer to inactive organizations, which results in a nil or erroneous task.assigned_to -- see a remedy in PR #16769.
  • acts_as_paranoid is also used to soft-delete records. It uses default_scope.
    • acts_as_paranoid is used by hearing_day.rb and worksheet_issue.rb -- see remedy in PR #16789.

Tests

Avoid comparing counts or array lengths

Tests which assert the length of arrays can be complicated to debug. Consider, for example:

it "returns the right number of items" do
  expect(subject.length).to eq 5
end

If it returns 4 instead, how will you debug?

Worse, it can mask bugs. Consider the following (hypothetical) method and test:

def positive_numbers(numbers)
  numbers.select {|n| n < 0 }
end

...

it "returns only positive numbers" do
  test_numbers = [-1, -2, 3, 4]
  expect(subject.count).to eq(2)
end

The test will pass, but it shouldn't! positive_numbers is only returning negative numbers.

It is so much better to test something more specific, e.g.:

it "returns only positive numbers" do
  positive_numbers = [3, 4]
  negative_numbers = [-1, -2]
  expect(subject(positive_numbers + negative_numbers).to match_array(positive_numbers)
end

Now you are testing the specific output, and if the test fails, you'll get a helpful diff of the unexpected results.