Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Could you please review the following class products?

It should tell me whether the product is taxable or imported. Name should indicate if the product is imported or certain keywords should tell that the product is non-taxable (chocolates, book, pills)

class Product
  NON_TAXABLE = [/chocolates/, /book/, /pills/]

  def initialize(product_name)
    @product_name = product_name
  end

  def is_taxable?
    taxable = false
    NON_TAXABLE.each { |x| taxable = x.match(@product_name) if taxable }
    !taxable
  end

  def is_imported?
    /imported/.match(@product_name)
  end
end
share|improve this question
add comment

2 Answers

Your is_taxable? function code does not seem to work. The issue is that taxable is never going to be true, so you'll never assign a new value. You want to write if !taxable instead. Your code becomes:

def is_taxable?
    taxable = false
    NON_TAXABLE.each { |x| taxable = x.match(@product_name) if !taxable }
    !taxable
end

It's still possible to make it simpler using Ruby functions on arrays.

def is_taxable?
  NON_TAXABLE.none? { |x| x.match(@product_name) }
end

This means: if my product is not among the tax-free products, then it is taxable.

share|improve this answer
    
I got confused many times by the "double not logic" (a taxable product is not is non taxable list) and by Ruby double not, but this should be a correct answer now. –  Quentin Pradet Feb 14 '13 at 8:46
    
you can drop !!, a successful match is a truish value. –  tokland Feb 14 '13 at 10:06
    
Yeah, but I would expect is_taxable? to return true or false, not a partial match or false. I don't know what's idiomatic in Ruby though. –  Quentin Pradet Feb 14 '13 at 10:20
    
It's definetely idiomatic to return only true/false for a method?. But I was talking about the !! in the block, Enumerable#none? will always return a boolean no matter what. –  tokland Feb 14 '13 at 10:21
    
Right, updated. –  Quentin Pradet Feb 14 '13 at 10:36
add comment

If you change the NON_TAXABLE constant from a bunch of regular expressions to one big regular expression to rule them all, like this:

NON_TAXABLE_RE = Regexp.union(%w(chocolates books pills))

Then the is_taxable? method (I prefer taxable?) would look like this:

def is_taxable?
  not NON_TAXABLE_RE.match(@product_name) #I like the explicit 'not'
end
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.