5
\$\begingroup\$

I'm formatting price string with the following method:

   def price_formatter(price)
     return if price.nil?
     formatted = '$'
     if price.length > 8 || price.length == 6
       price_separated = price.scan(/.{3}|.+/).join(',')
     elsif price.length == 8
       price_separated = price.insert(2, ',').insert(6, ',')
     elsif price.length == 7
       price_separated = price.insert(1, ',').insert(5, ',')
     elsif price.length == 5
       price_separated = price.insert(2, ',')
     elsif price.length == 4
       price_separated = price.insert(1, ',')
     else
       price_separated = price
     end
     formatted << price_separated
  end

Specs:

  it { expect(@listing.price_formatter('300000000')).to eq '$300,000,000' }
  it { expect(@listing.price_formatter('30000000')).to eq '$30,000,000' }
  it { expect(@listing.price_formatter('3000000')).to eq '$3,000,000' }
  it { expect(@listing.price_formatter('300000')).to eq '$300,000' }
  it { expect(@listing.price_formatter('30000')).to eq '$30,000' }
  it { expect(@listing.price_formatter('3000')).to eq '$3,000' }
  it { expect(@listing.price_formatter('300')).to eq '$300' }
  it { expect(@listing.price_formatter(nil)).to eq nil }

specs are passing, but I hate the long if statement and using a case is not accomplishing much either. What would be a good way to refactor the price_formatter method?

\$\endgroup\$
8
\$\begingroup\$

Code review-wise, I'd avoid long if-elsif chains and see about converting them to a case. Also, I'd move the final formatting to the end. Otherwise, not too bad except for the fact that there are much more concise ways to do this.

One is to notice that if you go from the end of the string backwards, there are fewer cases. In fact, just one:

def price_formatter(price)
  "$" + price.reverse.scan(/.{1,3}/).join(',').reverse
end

This reverses the string, looks for groups of up to 3 and joins them with commas, then reverses back.

You may have suspected that there might be a single regex that can do the trick. You would be correct:

def price_formatter(price)
  "$" + price.gsub(/(\d)(?=\d{3}+(\.\d*)?$)/, '\1,')
end

It's a little more obtuse in my opinion.

If you happen to be using Rails, I'd use the built-in helper:

def price_formatter(price)
  number_to_currency(price)
end

There are more options to number_to_currency, but the default is the output you want. In fact you are just recreating it, so you could refactor your method away completely.

Note you need to add return if price.nil? to handle the nil case in all the above.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I went with the regex option, thanks for taking the time to answer my question! \$\endgroup\$ – neo May 19 '15 at 19:39
  • \$\begingroup\$ Remarkably similar to my solution, here it is just for reference: def price_formatter(price) '$' + price.reverse.scan(/.{1,3}/).reverse.map(&:reverse).join(',') end but yours is a bit simpler. \$\endgroup\$ – Caridorc May 19 '15 at 19:39
  • \$\begingroup\$ I loved your Regexp answer! just a small matter, I would make the method accept numbers (integers and floats), maybe something like this: ("$% .2f" % price).gsub(/(\d)(?=\d{3}+(\.\d*)?$)/, '\1,') ... \$\endgroup\$ – Myst May 30 '15 at 19:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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