We aim to write code that is a pleasure to read, and we have a lot of opinions about how to do it well. Writing great code is an essential part of our programming culture, and we deliberately set a high bar for every code change anyone contributes. We care about how code reads, how code looks, and how code makes you feel when you read it.
We love discussing code. If you have questions about how to write something, or if you detect some smell you are not quite sure how to solve, please ask away to other programmers. A Pull Request is a great way to do this.
When writing new code, unless you are very familiar with our approach, try to find similar code elsewhere to look for inspiration.
In general, we prefer to use expanded conditionals over guard clauses.
# Bad
def todos_for_new_group
ids = params.require(:todolist)[:todo_ids]
return [] unless ids
@bucket.recordings.todos.find(ids.split(","))
end
# Good
def todos_for_new_group
if ids = params.require(:todolist)[:todo_ids]
@bucket.recordings.todos.find(ids.split(","))
else
[]
end
endThis is because guard clauses can be hard to read, especially when they are nested.
As an exception, we sometimes use guard clauses to return early from a method:
- When the return is right at the beginning of the method.
- When the main method body is not trivial and involves several lines of code.
def after_recorded_as_commit(recording)
return if recording.parent.was_created?
if recording.was_created?
broadcast_new_column(recording)
else
broadcast_column_change(recording)
end
endWe order methods in classes in the following order:
classmethodspublicmethods withinitializeat the top.privatemethods
We order methods vertically based on their invocation order. This helps us to understand the flow of the code.
class SomeClass
def some_method
method_1
method_2
end
private
def method_1
method_1_1
method_1_2
end
def method_1_1
# ...
end
def method_1_2
# ...
end
def method_2
method_2_1
method_2_2
end
def method_2_1
# ...
end
def method_2_2
# ...
end
endWe don't add a newline under visibility modifiers, and we indent the content under them.
class SomeClass
def some_method
# ...
end
private
def some_private_method_1
# ...
end
def some_private_method_2
# ...
end
endShould I call a method do_something or do_something!?
As a general rule, we only use ! for methods that have a correspondent counterpart without !. In particular, we don't use ! to flag destructive actions. There are plenty of destructive methods in Ruby and Rails that do not end with !.
In this codebase, we use bang methods for operations that perform the work synchronously (e.g., analyze!, generate!) when there's a corresponding async method (e.g., analyze_later, generate_later).
We model web endpoints as CRUD operations on resources (REST). When an action doesn't map cleanly to a standard CRUD verb, we introduce a new resource rather than adding custom actions.
# Bad
resources :cards do
post :close
post :reopen
end
# Good
resources :cards do
resource :closure
endIn general, we favor a vanilla Rails approach with thin controllers directly invoking a rich domain model. We don't use services or other artifacts to connect the two.
Invoking plain Active Record operations is totally fine:
class Cards::CommentsController < ApplicationController
def create
@comment = @card.comments.create!(comment_params)
end
endFor more complex behavior, we prefer clear, intention-revealing model APIs that controllers call directly:
class ContentsController < ApplicationController
def analyze
@content.pending!
@content.analyze_later
redirect_to @content, notice: "Re-analysis started..."
end
endWhen justified, it is fine to use services or form objects, but don't treat those as special artifacts:
Audio::Transcriber.new(file_path).transcribeAs a general rule, we write shallow job classes that delegate the logic itself to domain models:
- We typically use the suffix
_laterto flag methods that enqueue a job. - A common scenario is having a model class that enqueues a job that, when executed, invokes some method in that same class. In this case, we use the suffix
!for the synchronous method.
module Content::Analyzable
extend ActiveSupport::Concern
included do
after_create_commit :analyze_later
end
def analyze_later
AnalyzeContentJob.perform_later(self)
end
def analyze!
# ... actual analysis logic
end
end
class AnalyzeContentJob < ApplicationJob
def perform(content)
content.analyze!
end
endWe use concerns to extract cohesive functionality from models. Include the concern at the top of the model file:
class Content < ApplicationRecord
include Content::Analyzable
# ... rest of model
endUse included do blocks for hooks and associations that need to be defined in the context of the including class:
module Content::Analyzable
extend ActiveSupport::Concern
included do
has_many_attached :frames
after_create_commit :analyze_later
end
# ... methods
endPrefer scopes for reusable query logic:
class Entity < ApplicationRecord
scope :by_prominence, -> { order(prominence: :desc) }
scope :animals, -> { where(entity_type: "animal") }
scope :plants, -> { where(entity_type: "plant") }
endUse string-backed enums for readability in the database:
enum :status, %w[pending processing analyzed failed].index_by(&:itself)Use job-level error handling directives for retry and discard logic:
class GenerateRemixJob < ApplicationJob
retry_on Comfy::Client::ConnectionError, wait: 30.seconds, attempts: 5
discard_on Comfy::Client::APIError
def perform(remix)
remix.generate!
end
endWhen creating service objects, name them after what they do (noun form):
# Good
Audio::Transcriber.new(file_path).transcribe
Video::FrameExtractor.new(file_path).extract
# Bad
TranscribeAudio.new(file_path).call
ExtractVideoFrames.new(file_path).callUse JavaScript's native # prefix for private methods and instance variables. Everything that isn't part of the public API (actions, target callbacks, lifecycle) should be private.
export default class extends Controller {
#timer
#fetchController
// Public action
submit() {
if (this.#dirty) this.#save()
}
// Private
#save() {
// ...
}
get #dirty() {
return !!this.#timer
}
}Organize controllers in this order: static declarations, lifecycle, actions, public methods, private methods. Separate sections with a comment.
export default class extends Controller {
static targets = [ "input", "item" ]
static values = { ... }
static classes = [ "active" ]
// Lifecycle
initialize() { }
connect() { }
disconnect() { }
// Actions
toggle() { }
close() { }
// Public
selectItem(item) { }
// Private
#clearSelection() { }
}Use initialize() for one-time setup that must happen before the value observer runs (e.g., reading initial DOM state, creating debounced functions). Use connect() for setup that depends on targets and values being ready. Use disconnect() for cleanup.
initialize() {
this.filter = debounce(this.filter.bind(this), 100)
}Use target callbacks (targetConnected/targetDisconnected) for dynamic elements instead of manually managing event listeners on targets:
imageTargetConnected(element) {
element.addEventListener("click", this.#handleImageClick)
}
imageTargetDisconnected(element) {
element.removeEventListener("click", this.#handleImageClick)
}
#handleImageClick = (event) => {
event.preventDefault()
this.#open(event.currentTarget)
}Prefer Stimulus actions declared in HTML over manual addEventListener. For document or window-level events, use the @document or @window modifier in data-action:
data-action="click@window->dialog#closeOnClickOutside"When manual listeners are truly necessary (e.g., for events Stimulus can't bind), use arrow function class fields so the reference is stable for removal:
#handleClick = (event) => { ... }Use a consistent pattern and naming for outside-click handlers:
closeOnClickOutside({ target }) {
if (!this.element.contains(target)) this.close()
}Use a key handler object instead of a switch statement:
#keyHandlers = {
ArrowDown(event) {
this.#selectNext()
event.preventDefault()
},
ArrowUp(event) {
this.#selectPrevious()
event.preventDefault()
},
Enter(event) {
if (event.isComposing) return
this.#confirmSelection(event)
}
}
navigate(event) {
this.#keyHandlers[event.key]?.call(this, event)
}Create debounced versions of methods in initialize() using a shared debounce helper. Bind the method so it can be used as an action directly:
initialize() {
this.search = debounce(this.search.bind(this), 200)
}Action methods describe what they do, not what event triggered them. Never prefix actions with on — names like onClick, onInput, onKeydown describe the event, not the intent. Instead, name the action after the behavior it performs:
// Bad — describes the event
onInput() { ... }
onClick() { ... }
onKeydown() { ... }
// Good — describes the behavior
filter() { ... }
select() { ... }
navigate() { ... }Target names are nouns: input, item, label, dialog. Value names describe configuration: reverseOrder, selectionAttribute, autoScroll.
Extract magic numbers and timing values into constants at the top of the file:
const AUTOSAVE_INTERVAL = 3000
const DEBOUNCE_DELAY = 100Use getters for derived state instead of storing redundant instance variables:
get #dirty() {
return !!this.#timer
}
get #visibleItems() {
return this.itemTargets.filter(item => !item.hidden)
}Use DOM APIs (cloneNode, appendChild, textContent, classList, toggleAttribute) rather than building HTML strings with template literals. Clone <template> elements for repeating structures. Use textContent instead of innerHTML when setting text to avoid XSS risks.
All fixtures are loaded globally via fixtures :all in test_helper.rb. Never declare fixtures in individual test files.
When adding new fixture files, make sure all associations reference labels that exist in other fixture files — broken references cause foreign key violations when all fixtures load together.
Don't test private methods directly. Test them through the public interface that exercises them.
Prefix test names with #method_name for instance methods and .method_name for class methods:
test "#excerpt returns the first 300 characters of the plain text content" do
test ".published returns only published entryables" doFollow a setup–action–assertion pattern with blank lines between each phase:
test "#published= writes through to the entry" do
article = articles(:misguided_mark)
article.published = false
assert_equal false, article.entry.published
endInclude a descriptive message on assertions to explain intent, especially when the assertion alone doesn't make the failure obvious:
assert published_articles.all?(&:published?), "Should only return published articles"Prefer fixture data for test setup. Only create records inline when the test needs specific state that doesn't exist in fixtures:
# Prefer loading fixtures
article = articles(:misguided_mark)
# Create inline only when you need specific state
Article.create!(title: "Unpublished Article", body: "Content", published: false, tags: "people")Mirror the app/models/ structure under test/models/. Test concerns in their own file (e.g., test/models/entryable_test.rb for the Entryable concern).