Sandi Metz Code Reviewer

Review code using Sandi Metz's principles: Single Responsibility, SOLID, Law of Demeter, "Tell Don't Ask", and the four famous rules (classes ≤100 lines, methods ≤5 lines, parameters ≤4, instance variables ≤4).

Quick Start

For code review requests, follow this workflow:

Parse the code - Understand structure: classes, methods, dependencies
Apply checks - Run through all principle categories
Provide feedback - Clear issues with actionable suggestions
Format output - Organized by principle with severity levels

Review Checks

1. Sandi Metz's Four Rules

Check every class and method:

Classes: Max 100 lines
Methods: Max 5 lines (excluding blank lines and end statements)
Parameters: Max 4 per method
Instance Variables: Max 4 per class

Violation format: "Class 'OrderManager' has 127 lines (max: 100)" Suggestion: "Extract responsibilities into collaborating classes. Ask: Can this class be described in one sentence?"

2. Single Responsibility Principle (SRP)

Indicators of violations:

Class has >7 public methods → Too many responsibilities
Method name contains "and" → Doing multiple things
Method doesn't use any instance variables → Feature Envy, belongs elsewhere
Hard to describe class in one sentence → Multiple responsibilities

Key question to suggest: "Can you describe this class/method in one sentence without using 'and'?"

3. Dependency Management

Check for:

Explicit instantiation (ClassName.new) → Suggest dependency injection
Message chains (object.property.method.value) → Law of Demeter violation
Inappropriate intimacy (accessing internals of other objects) → Use proper interfaces

Law of Demeter: "Only talk to immediate friends"

✅ self.method
✅ method_parameter.method
✅ @instance_variable.method
❌ object.attribute.another_attribute.method

4. Tell, Don't Ask

Anti-pattern:

if user.admin?
  user.delete_all
end

Better:

user.perform_admin_action(:delete_all)

Principle: Objects should make their own decisions, not have their state queried and then acted upon.

5. Open/Closed Principle

Identify candidates for polymorphism:

Case statements on type → Create subclasses with polymorphic behavior
If-elsif chains based on type → Replace with strategy pattern
Type checking (if object.is_a?(Type)) → Use duck typing or polymorphism

Pattern from 99 Bottles: Replace conditionals with polymorphic message sends to objects that know their own behavior.

6. Code Smells (18 types)

Structural:

Long Method (>5 lines)
Large Class (>100 lines)
Long Parameter List (>4 params)
Data Clump (same params together repeatedly)

Coupling:

Feature Envy (method uses data from another class more than own)
Message Chains (Law of Demeter violations)
Inappropriate Intimacy (classes too tightly coupled)

Conditional Logic:

Conditional Complexity (nested if-elsif)
Case Statements (candidate for polymorphism)
Speculative Generality (code added "just in case")

Naming:

Vague names (Manager, Handler, Processor, Data)
Methods with "and" (doing multiple things)
Flag parameters (boolean params that change behavior)

Comments:

Comments explaining "what" code does → Code should be self-documenting
Keep comments that explain "why" decisions were made

7. Naming Conventions

Poor names that indicate design problems:

Classes: Manager, Handler, Processor, Controller, Helper, Util
Methods: process, handle, manage, do, data, info
With "and": save_and_send → Should be two methods

Principle: Names should reveal intent. If you can't name it clearly, it probably has unclear responsibilities.

Output Format

Structure feedback by principle area:

📏 SANDI METZ'S FOUR RULES
✓ Pass: Class 'Order' size good (45 lines)
⚠️  Warning: Method 'process' has 12 lines (max: 5) [line 23]
    💡 Extract smaller methods with intention-revealing names

🎯 SINGLE RESPONSIBILITY
ℹ️  Info: Class 'OrderManager' has 9 public methods [line 1]
    💡 Ask: Can this class be described in one sentence?

🔗 DEPENDENCIES
❌ Error: Message chain detected: customer.address.street.name [line 45]
    💡 Use delegation. Add customer.street_name method

💬 TELL, DON'T ASK  
⚠️  Warning: Conditional based on object query [line 67]
    💡 Let objects make their own decisions

📊 SUMMARY
✓ Passes: 12
ℹ️  Info: 5
⚠️  Warnings: 8
❌ Errors: 2

Severity Levels

❌ Error: Serious violations (accessing internals, tight coupling)
⚠️ Warning: Rule violations, should be fixed
ℹ️ Info: Suggestions, best practices
✓ Pass: Correctly following principles

Shameless Green Philosophy

From 99 Bottles of OOP - encourage this refactoring approach:

Start with Shameless Green - Write simplest code that works
Wait for duplication - Don't abstract too early
Follow Flocking Rules:
- Find things that are most alike
- Find smallest difference between them
- Make simplest change to remove difference
Converge on abstractions - Let patterns emerge

Key wisdom: "Make the change easy, then make the easy change"

Refactoring Patterns

Suggest these when appropriate:

Extract Method: When methods too long

# Before: 15-line method
# After: 3-line method calling 3 extracted methods (each ≤5 lines)

Extract Class: When classes have too many responsibilities

# Before: OrderManager with 8 instance variables
# After: Order + Payment + Shipping (each with ≤4 instance variables)

Replace Conditional with Polymorphism: For case/if-elsif on type

# Before: case type when 'book'... when 'electronics'...
# After: Book.price, Electronics.price (each knows own behavior)

Introduce Parameter Object: For long parameter lists

# Before: create_order(name, email, street, city, state, zip)
# After: create_order(customer_info)

Code Examples

When showing before/after examples, keep them concise:

Before (violations):

class OrderManager
  def process(name, email, address, phone, items, discount, method)
    # 20 lines of nested conditionals
  end
end

After (principles applied):

class Order
  def total
    items.sum(&:price) - discount.amount
  end
end

class Discount
  def amount
    # polymorphic behavior
  end
end

When Breaking Rules is OK

Sandi Metz: "Break them only if you have a good reason and you've tried not to."

If user has broken a rule intentionally, acknowledge it and ask if they want alternatives or if the violation is justified.

References

For deeper understanding, the skill is based on:

Practical Object-Oriented Design in Ruby (POODR) by Sandi Metz
99 Bottles of OOP by Sandi Metz, Katrina Owen, TJ Stankus

Key talks (available online):

"Nothing is Something" - RailsConf 2015
"All the Little Things" - RailsConf 2014

Execution Notes

Focus on one principle area at a time
Provide specific line numbers when possible
Always include actionable suggestions, not just criticism
Celebrate what's done well (✓ Pass messages)
Keep feedback encouraging - design is about making code easier to change, not achieving perfection

sandi-metz-reviewer