Skip to Content
Course content

173: Code Review Checklist for Idiomatic Ruby

Click on the "Edit" button in the top corner of the screen to edit your slide content.

I've spent a lot of my career reviewing Pull Requests, and there is a specific kind of friction that happens when a developer who is brilliant at logic—but new to Ruby—submits their first few features. The code works. The tests pass. But it looks like Java or C# written with Ruby syntax. It's what I call "Naive Ruby." It gets the job done, but it ignores the language's actual strengths, making the codebase harder to maintain because it's more verbose than it needs to be.

When I'm reviewing your code, I'm not just looking for bugs; I'm looking for intent. Idiomatic Ruby is designed to read like a sentence. When you deviate from that, you're forcing the next developer to mentally "compile" your logic rather than just reading it.

Stopping the manual push

One of the most common patterns I see is the "accumulator" pattern. You initialize an empty array, loop through a collection, check a condition, and manually push the matching elements into that array. It looks something like this:

# The Naive Way
premium_users = []
users.each do |user|
  if user.spent_amount > 1000 && user.active?
    premium_users << user
  end
end

This isn't "wrong," but it's noisy. You're managing the state of premium_users manually. In a professional Ruby codebase, I'll almost always ask you to replace this with select. We aren't just iterating; we are filtering.

# The Idiomatic Way
premium_users = users.select { |user| user.spent_amount > 1000 && user.active? }

The trade-off here is clarity. With select, the intent is declared upfront. I don't have to scan the block to figure out that you're building a filtered list; the method name tells me exactly what the outcome is. If you find yourself initializing an empty array just to fill it during a loop, you're probably using the wrong Enumerable method.

Reducing the noise of existence checks

Ruby handles nil in a way that can lead to some truly ugly "guard" chains. I often see developers writing long strings of && checks to avoid the dreaded NoMethodError when digging through nested objects.

# The Naive Way
if order && order.customer && order.customer.address && order.customer.address.zip_code
  send_package(order.customer.address.zip_code)
end

Reading this feels like walking through a minefield. It's tedious and distracts from the actual goal: getting the zip code. This is where the safe navigation operator (&.) becomes your best friend. It allows you to attempt a method call, and if any link in the chain is nil, the whole expression gracefully returns nil instead of crashing.

# The Idiomatic Way
if zip = order&.customer&.address&.zip_code
  send_package(zip)
end

I combined the navigation with an assignment inside the if statement here. This prevents us from having to call the entire chain a second time inside the block. It's leaner, safer, and significantly easier on the eyes.

Replacing flags with predicates

Finally, let's talk about "flagging." I see a lot of logic where a developer creates a boolean variable, loops through a collection, and flips that boolean to true the moment a condition is met. It's a very imperative style of programming.

# The Naive Way
has_expired_subscriptions = false
subscriptions.each do |sub|
  if sub.end_date < Date.today
    has_expired_subscriptions = true
    break
  end
end

This is a lot of ceremony for a simple question: "Are there any expired subscriptions?" Ruby gives us any? and all? for this exact reason. These methods are not only more concise but are optimized to stop iterating the moment the result is determined.

# The Idiomatic Way
has_expired_subscriptions = subscriptions.any? { |sub| sub.end_date < Date.today }

Or, if you have a predicate method defined on the subscription model, you can use the shorthand symbol-to-proc syntax:

# Even Better
has_expired_subscriptions = subscriptions.any?(&:expired?)

When you write code like this, you stop telling Ruby how to do the work (initialize variable, loop, check, assign, break) and start telling it what you want (do any of these match this criteria?). That shift in perspective is what separates a Ruby developer from someone who is just writing another language in Ruby.




📋 Practical Task

Refactoring the Order Processing Module

You've been handed a legacy piece of code used to analyze customer orders. While it works, it's written in a very non-idiomatic style that makes the team shudder during code reviews. Your task is to refactor the OrderAnalyzer class to use idiomatic Ruby.

Requirements:

  • Replace the manual array accumulation in high_value_orders with a select block.
  • Replace the manual boolean flag in has_urgent_orders? with an any? call.
  • Replace the multi-step nil checks in customer_email with the safe navigation operator (&.).
class OrderAnalyzer
  def initialize(orders)
    @orders = orders
  end

  def high_value_orders
    result = []
    @orders.each do |order|
      if order.total > 500
        result << order
      end
    end
    result
  end

  def has_urgent_orders?
    urgent = false
    @orders.each do |order|
      if order.priority == 'high' && order.status == 'pending'
        urgent = true
        break
      end
    end
    urgent
  end

  def customer_email(order)
    if order && order.customer && order.customer.contact && order.customer.contact.email
      return order.customer.contact.email
    else
      return nil
    end
  end
end
Rating
0 0

There are no comments for now.

to be the first to leave a comment.