Idioms, Style and Clean Code

# frozen_string_literal: true

require "shelf"

module Shelf
  # Guard clauses reject bad input first, so the main path is not nested.
  module Lending
    module_function

    def due_date(loan_days, from: Time.now)
      unless loan_days.is_a?(Integer) && loan_days.positive?
        raise ArgumentError, "loan_days must be a positive Integer, got #{loan_days.inspect}"
      end

      from + (loan_days * 86_400)
    end

    def titles(books)
      return [] if books.nil?

      Array(books).map(&:title)
    end

    # fetch states what a missing key means instead of letting nil travel on.
    def setting(options, name)
      options.fetch(name) { raise Shelf::Error, "missing setting: #{name}" }
    end
  end
end

Shelf::Lending.due_date(14, from: Time.utc(2026, 1, 1)) # => 2026-01-15 00:00:00 UTC

begin
  Shelf::Lending.due_date(0)
rescue ArgumentError => e
  e.message # => "loan_days must be a positive Integer, got 0"
end

author = Shelf::Author.new("Frank Herbert")
book = Shelf::Book.new(isbn: Shelf::Isbn.new("978-0-306-40615-7"), title: "Dune", author:)
Shelf::Lending.titles(nil) # => []
Shelf::Lending.titles(book) # => ["Dune"]
Shelf::Lending.titles([book, book]) # => ["Dune", "Dune"]

options = { loan_days: 14, reminders: { email: false } }
Shelf::Lending.setting(options, :loan_days) # => 14
options[:renewals] # => nil
options.dig(:reminders, :email) # => false
options.dig(:reminders, :sms)&.upcase # => nil
options[:reminders][:email] || "default" # => "default"
options.fetch(:reminders).fetch(:email, "default") # => false

begin
  Shelf::Lending.setting(options, :renewals)
rescue Shelf::Error => e
  e.message # => "missing setting: renewals"
end

begin
  options.fetch(:renewals)
rescue KeyError => e
  e.message # => "key not found: :renewals"
end

# frozen_string_literal: true

require "shelf"

rows = [["The Dispossessed", "Ursula K. Le Guin", "978-0-306-40615-7", 1974],
        ["A Wizard of Earthsea", "Ursula K. Le Guin", "978-0-14-131026-8", 1968],
        ["Dune", "Frank Herbert", "978-0-441-17271-9", 1965],
        ["Neuromancer", "William Gibson", "978-0-553-38256-3", 1984]]
books = rows.map do |title, name, digits, _year|
  Shelf::Book.new(isbn: Shelf::Isbn.new(digits), title:, author: Shelf::Author.new(name))
end
years = rows.to_h { |title, _name, _digits, year| [title, year] }

# C-style: index, accumulator and mutation all visible.
def ursula_titles(books)
  result = []
  i = 0
  while i < books.size
    result << books[i].title.upcase if books[i].author.name.start_with?("Ursula")
    i += 1
  end
  result
end

ursula_titles(books) # => ["THE DISPOSSESSED", "A WIZARD OF EARTHSEA"]

# Enumerable: say what you want. Same result, no counter, no accumulator.
books.select { it.author.name.start_with?("Ursula") }.map { it.title.upcase }
# => ["THE DISPOSSESSED", "A WIZARD OF EARTHSEA"]

# filter_map: select and map in one pass.
books.filter_map { it.title.upcase if it.author.name.start_with?("Ursula") }
# => ["THE DISPOSSESSED", "A WIZARD OF EARTHSEA"]

# Symbol#to_proc for a single message.
books.map(&:title) # => ["The Dispossessed", "A Wizard of Earthsea", "Dune", "Neuromancer"]

books.find { it.title == "Dune" }&.author&.name # => "Frank Herbert"
books.any? { years.fetch(it.title) < 1970 } # => true
books.count { years.fetch(it.title) < 1970 } # => 2
books.group_by { it.author.name }.transform_values(&:size)
# => {"Ursula K. Le Guin" => 2, "Frank Herbert" => 1, "William Gibson" => 1}
books.map { it.author.name }.tally.max_by(&:last) # => ["Ursula K. Le Guin", 2]
books.sum { years.fetch(it.title) } # => 7891
books.partition { years.fetch(it.title) < 1970 }.map { |part| part.map(&:title) }
# => [["A Wizard of Earthsea", "Dune"], ["The Dispossessed", "Neuromancer"]]
books.each_with_object(Hash.new { |hash, key| hash[key] = [] }) do |book, by_decade|
  by_decade[years.fetch(book.title) / 10 * 10] << book.title
end
# => {1970 => ["The Dispossessed"], 1960 => ["A Wizard of Earthsea", "Dune"], 1980 => ["Neuromancer"]}
books.each_slice(2).map { |pair| pair.map(&:title) }
# => [["The Dispossessed", "A Wizard of Earthsea"], ["Dune", "Neuromancer"]]

# Numbered counting loops belong to Integer, not to a while loop.
3.times.map { it * 2 } # => [0, 2, 4]
1.upto(3).to_a # => [1, 2, 3]
3.downto(1).to_a # => [3, 2, 1]
(1..10).step(3).to_a # => [1, 4, 7, 10]

# Where an explicit loop is right: the algorithm is the loop.
def collatz_steps(number)
  steps = 0
  until number == 1
    number = number.even? ? number / 2 : (3 * number) + 1
    steps += 1
  end
  steps
end

collatz_steps(6) # => 8

# frozen_string_literal: true

require "shelf"

module Shelf
  # The value of a method is its last expression; return is for leaving early.
  class Bookcase
    attr_reader :books

    def initialize = @books = []

    def add(book)
      @books << book
      self
    end

    def count = @books.size

    # Assignment methods always evaluate to the right-hand side, whatever they return.
    def label=(value)
      @label = value.to_s.strip
      :ignored
    end

    def label = @label || "unlabelled"

    def empty_report
      puts "nothing to report" if books.empty?
    end
  end
end

author = Shelf::Author.new("Frank Herbert")
book = Shelf::Book.new(isbn: Shelf::Isbn.new("978-0-441-17271-9"), title: "Dune", author:)
shelf = Shelf::Bookcase.new

shelf.add(book).add(book).count # => 2
echoed = shelf.label = "  fiction "
echoed.length # => 10
shelf.label # => "fiction"
shelf.send(:label=, " x ") # => :ignored
shelf.empty_report # => nil
Shelf::Bookcase.new.empty_report # prints: nothing to report, returns nil

titles = shelf.books
              .map(&:title)
              .uniq
              .join(", ")
titles.upcase # => "DUNE"

# frozen_string_literal: true

require "shelf"

module Shelf
  # A small immutable value object: equality, ordering and a readable to_s for free.
  LoanPeriod = Data.define(:days) do
    include Comparable

    def initialize(days:)
      raise ArgumentError, "days must be positive, got #{days.inspect}" unless days.is_a?(Integer) && days.positive?

      super
    end

    def <=>(other) = other.is_a?(LoanPeriod) ? days <=> other.days : nil
    def +(other) = with(days: days + other.days)
    def to_s = "#{days} #{days == 1 ? 'day' : 'days'}"
  end

  # A fluent builder: every step returns self, so calls chain.
  class LoanRequest
    def initialize = @attributes = {}

    def for_book(book)
      @attributes[:book] = book
      self
    end

    def for_period(period)
      @attributes[:period] = period
      self
    end

    def build = Loan.new(@attributes.fetch(:book), state: :lent)
  end
end

week = Shelf::LoanPeriod.new(days: 7)
fortnight = Shelf::LoanPeriod.new(days: 14)

week.eql?(Shelf::LoanPeriod.new(days: 7)) # => true
week.between?(week, fortnight) # => true
[week, fortnight].max # => #<data Shelf::LoanPeriod days=14>
[fortnight, week].min # => #<data Shelf::LoanPeriod days=7>
[week, fortnight].reduce(:+) # => #<data Shelf::LoanPeriod days=21>
week.with(days: 1).to_s # => "1 day"
fortnight.clamp(week, week).to_s # => "7 days"
"Loan for #{week}".delete_prefix("Loan for ") # => "7 days"

begin
  week.instance_variable_set(:@days, 1)
rescue FrozenError => e
  e.message # => "can't modify frozen Shelf::LoanPeriod: #<data Shelf::LoanPeriod days=7>"
end

begin
  Shelf::LoanPeriod.new(days: 0)
rescue ArgumentError => e
  e.message # => "days must be positive, got 0"
end

author = Shelf::Author.new("Frank Herbert")
book = Shelf::Book.new(isbn: Shelf::Isbn.new("978-0-441-17271-9"), title: "Dune", author:)
loan = Shelf::LoanRequest.new.for_book(book).for_period(week).build
loan.describe # => "Dune is lent"

case book
in { title:, author: { name: } }
  "#{title} / #{name}" # => "Dune / Frank Herbert"
end

# frozen_string_literal: true

require "shelf"

# Small idioms that make Ruby read like the problem.
author = Shelf::Author.new("Ursula K. Le Guin")
isbn = Shelf::Isbn.new("978-0-306-40615-7")
title = "The Dispossessed"

# Hash and keyword shorthand (3.1): name the value once.
Shelf::Book.new(isbn:, title:, author:).to_s # => "The Dispossessed by Ursula K. Le Guin"

# Word and symbol lists.
%w[available lent overdue].map(&:to_sym) # => [:available, :lent, :overdue]
%i[available lent overdue].include?(:lent) # => true

# Configuration with a default: ENV.fetch with a block states the policy.
loan_days = Integer(ENV.fetch("SHELF_LOAN_DAYS", "14"), exception: false) || 14
loan_days.to_s # => "14"

# Building text: a heredoc for prose, format for numbers, << on a mutable buffer in loops.
fine = 1750
report = +"Fines\n"
[["Ana", fine], ["Ben", 0]].each do |name, cents|
  report << format("%-5<name>s %<amount>7.2f\n", name:, amount: cents / 100.0)
end
report.lines.map(&:chomp) # => ["Fines", "Ana     17.50", "Ben      0.00"]

help = <<~TEXT
  Usage: shelf [options]
    -v  print the version
TEXT
help.lines.size # => 2

# Memoization with ||= is fine for values that are never nil or false.
class Report
  def initialize(catalog) = @catalog = catalog

  def titles = @titles ||= @catalog.map(&:title).sort
end

catalog = Shelf::Catalog.new.add(Shelf::Book.new(isbn:, title:, author:))
reports = Report.new(catalog)
reports.titles.equal?(reports.titles) # => true

# then and tap: a pipeline and a peek, without temporary variables.
"  the dispossessed ".strip.then { |text| text.split.map(&:capitalize).join(" ") } # => "The Dispossessed"
sizes = []
[3, 1, 2].sort.tap { |sorted| sizes << sorted.size }.first # => 1
sizes.first # => 3

# Block-local defaults with Hash.new and a block; tally for counting.
%w[a b a].tally # => {"a" => 2, "b" => 1}
Hash.new { |hash, key| hash[key] = key.to_s * 2 }[:ab] # => "abab"

# Comparable#clamp keeps a value within bounds, which is clearer than min and max.
120.clamp(0, 100) # => 100
-5.clamp(0..) # => 0

# frozen_string_literal: true

module Shelf
  # Works, but nobody dares to touch it.
  class FineCalculator
    def self.calc(days, yrs, dmg)
      fine = 0
      if days > 3
        if yrs >= 5 && !dmg
          fine = 0
        else
          fine = (days - 3) * 25
          fine = 1000 if fine > 1000
        end
      end
      fine
    end
  end
end

# frozen_string_literal: true

require "minitest/autorun"

# The first four tests pin down what the code does today (characterization tests).
class FineCalculatorTest < Minitest::Test
  def test_no_fine_inside_the_grace_period
    assert_equal 0, Shelf::FineCalculator.calc(3, 1, false)
  end

  def test_charges_per_day_after_the_grace_period
    assert_equal 175, Shelf::FineCalculator.calc(10, 1, false)
  end

  def test_fine_is_capped
    assert_equal 1000, Shelf::FineCalculator.calc(100, 1, false)
  end

  def test_loyal_members_are_waived_unless_the_book_is_damaged
    assert_equal 0, Shelf::FineCalculator.calc(10, 8, false)
    assert_equal 175, Shelf::FineCalculator.calc(10, 8, true)
  end

  # Added during the refactoring, for the new keyword API: red until the refactoring is done.
  def test_call_matches_calc
    assert_equal 175, Shelf::FineCalculator.call(days_late: 10, member_since_years: 1)
  end

  def test_new_is_no_longer_public
    assert_raises(NoMethodError) { Shelf::FineCalculator.new(1, 1, false) }
  end
end

Run options: --seed 1

# Running:

..FE..

Finished in 0.000325s, 18461.5368 runs/s, 18461.5368 assertions/s.

  1) Failure:
FineCalculatorTest#test_new_is_no_longer_public [fine_test.rb:30]:
[NoMethodError] exception expected, not
Class: <ArgumentError>
Message: <"wrong number of arguments (given 3, expected 0)">
---Backtrace---
fine_test.rb:30:in 'BasicObject#initialize'
fine_test.rb:30:in 'block in FineCalculatorTest#test_new_is_no_longer_public'
---------------

  2) Error:
FineCalculatorTest#test_call_matches_calc:
NoMethodError: undefined method 'call' for class Shelf::FineCalculator
    fine_test.rb:26:in 'FineCalculatorTest#test_call_matches_calc'

6 runs, 6 assertions, 1 failures, 1 errors, 0 skips

# frozen_string_literal: true

module Shelf
  # The same behaviour, split into named steps, with a keyword API.
  class FineCalculator
    GRACE_DAYS = 3
    CENTS_PER_DAY = 25
    CAP_CENTS = 1_000
    LOYAL_YEARS = 5

    def self.call(days_late:, member_since_years:, damaged: false)
      new(days_late, member_since_years, damaged).cents
    end

    # The old entry point stays for one release and says so.
    def self.calc(days, years, damaged)
      warn "FineCalculator.calc is deprecated, use FineCalculator.call", category: :deprecated, uplevel: 1
      call(days_late: days, member_since_years: years, damaged:)
    end

    private_class_method :new

    def initialize(days_late, member_since_years, damaged)
      @days_late = days_late
      @member_since_years = member_since_years
      @damaged = damaged
    end

    def cents
      return 0 if waived?

      [chargeable_days * CENTS_PER_DAY, CAP_CENTS].min
    end

    private

    def waived? = chargeable_days <= 0 || (@member_since_years >= LOYAL_YEARS && !@damaged)

    def chargeable_days = @days_late - GRACE_DAYS
  end
end

fine_test.rb:16: warning: FineCalculator.calc is deprecated, use FineCalculator.call
Run options: --seed 1

# Running:

.fine_test.rb:12: warning: FineCalculator.calc is deprecated, use FineCalculator.call
...fine_test.rb:20: warning: FineCalculator.calc is deprecated, use FineCalculator.call
fine_test.rb:21: warning: FineCalculator.calc is deprecated, use FineCalculator.call
.fine_test.rb:8: warning: FineCalculator.calc is deprecated, use FineCalculator.call
.

Finished in 0.000356s, 16853.9310 runs/s, 19662.9195 assertions/s.

6 runs, 7 assertions, 0 failures, 0 errors, 0 skips

# frozen_string_literal: true

class Reservation
  def is_active?(status)
    if status != nil
      if status == :lent
        return true
      else
        return false
      end
    end
  end

  def titles(books)
    result = []
    for book in books
      result << book.title
    end
    result
  end

  def notice(overdue)
    unless overdue
      "ok"
    else
      "late"
    end
  end

  def present?(value) = !!value
end

4:7: C: Naming/PredicatePrefix: Rename `is_active?` to `active?`.
5:5: C: [Correctable] Style/GuardClause: Use a guard clause (`return unless status != nil`) instead of wrapping the code inside a conditional expression.
5:8: C: [Correctable] Style/NonNilCheck: Prefer `!status.nil?` over `status != nil`.
6:7: C: [Correctable] Style/GuardClause: Use a guard clause (`return true if status == :lent`) instead of wrapping the code inside a conditional expression.
7:9: C: [Correctable] Style/RedundantReturn: Redundant `return` detected.
9:9: C: [Correctable] Style/RedundantReturn: Redundant `return` detected.
16:5: C: [Correctable] Style/For: Prefer `each` over `for`.
23:5: C: [Correctable] Style/UnlessElse: Do not use `unless` with `else`. Rewrite these with the positive case first.