HiveBrain v1.2.0
Get Started
← Back to all entries
debugrubyMinor

Good approach to raise an exception

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
approachexceptionraisegood

Problem

I have class House and module Lockable. Locking and unlocking House should reflect the real world, so you can't lock twice.

Do you think this is a good approach to using a module and raising an exception?

module Lockable
  attr_reader :locked

  def lock!
    raise StandardError, "Already locked" if @locked == true
    @locked = true
  end

  def unlock!
    raise StandardError, "Already unlocked" if @locked == false
    @locked = false
  end
end

class House
  include Lockable
end


Usage:

house = House.new
house.lock!
house.unlock!
house.unlock! # raise an exception

Solution

I would define domain specific error classes like so:

module Lockable
  Error           = Class.new StandardError
  AlreadyLocked   = Class.new Error
  AlreadyUnlocked = Class.new Error

  def lock!
    raise AlreadyLocked if locked?
    @locked = true
  end

  def unlock!
    raise AlreadyUnlocked unless locked?
    @locked = false
  end

  def locked?
    # unless already defined, this assumes an
    # initial unlocked state (!!nil == false)
    !!@locked
  end
end


By having a common ancestor (LockedError), users can simply catch that one if they don't care about the inner state of the lockable object:

begin
  house.lock!
  house.lock!
rescue Lockable::Error
  # meh...
rescue StandardError
  # $! needs handling
end


On the other hand, if I try to lock an already locked lock, the key just won't move (and the lock doesn't blow up in my face). So, a more realistic reflection of the world would be this implementation:

module Lockable
  def lock!
    @locked = true
  end

  def unlock!
    @locked = false
  end
end

Code Snippets

module Lockable
  Error           = Class.new StandardError
  AlreadyLocked   = Class.new Error
  AlreadyUnlocked = Class.new Error

  def lock!
    raise AlreadyLocked if locked?
    @locked = true
  end

  def unlock!
    raise AlreadyUnlocked unless locked?
    @locked = false
  end

  def locked?
    # unless already defined, this assumes an
    # initial unlocked state (!!nil == false)
    !!@locked
  end
end
begin
  house.lock!
  house.lock!
rescue Lockable::Error
  # meh...
rescue StandardError
  # $! needs handling
end
module Lockable
  def lock!
    @locked = true
  end

  def unlock!
    @locked = false
  end
end

Context

StackExchange Code Review Q#58869, answer score: 6

Revisions (0)

No revisions yet.