views:

34

answers:

1

Imagine I have a class like this:

class A
   attr_accessor :a
   attr_accessor :b

   def check
      raise "a must meet some condition" if !condition(@a)
      raise "b must meet some condition" if !condition(@b)
   end

   def do_some_work
      check()
      # more work here
   end

   def do_some_more_work
      check()
      # other work here
   end
end

Is it bad practice to have the preconditions for the accessors inside another method, and not the current one?

+3  A: 

It would be better to check when the assignment takes place. Fail fast.

class A
  attr_reader :a, :b

  def a=(new_a)
    raise "a must meet some condition" if !condition(@a)
    @a = new_a
  end

  def b=(new_b)
    raise "b must meet some condition" if !condition(@b)
    @b = new_b
  end

  def do_some_work
    # Do the work, knowing @a and @b are valid
  end

  # ...
end
molf
What if condition includes checking they are not nil?
Geo
@Geo, if they should not be `nil`, you probably want to pass them to the constructor. For example: `A.new(:a => "some value", :b => "another value")` or `A.new("some value", "another value")`.
molf