3
\$\begingroup\$

This 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).

Could you please review the following class products?

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
\$\endgroup\$

2 Answers 2

Reset to default
5
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Feb 14, 2013 at 8:46
  • \$\begingroup\$ you can drop !!, a successful match is a truish value. \$\endgroup\$ Commented Feb 14, 2013 at 10:06
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Feb 14, 2013 at 10:20
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Feb 14, 2013 at 10:21
  • \$\begingroup\$ Right, updated. \$\endgroup\$ Commented Feb 14, 2013 at 10:36
0
\$\begingroup\$

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
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.