patternrubyMinor
Formatting a price string
Viewed 0 times
formattingstringprice
Problem
I'm formatting price string with the following method:
Specs:
%%CODEBLOCK_1%%
specs are passing, but I hate the long
def price_formatter(price)
return if price.nil?
formatted = '
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?
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
endSpecs:
%%CODEBLOCK_1%%
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?Solution
Code review-wise, I'd avoid long
One is to notice that if you go from the end of the string backwards, there are fewer cases. In fact, just one:
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:
It's a little more obtuse in my opinion.
If you happen to be using Rails, I'd use the built-in helper:
There are more options to
Note you need to add
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
endThis 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,')
endIt'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)
endThere 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.Code Snippets
def price_formatter(price)
"$" + price.reverse.scan(/.{1,3}/).join(',').reverse
enddef price_formatter(price)
"$" + price.gsub(/(\d)(?=\d{3}+(\.\d*)?$)/, '\1,')
enddef price_formatter(price)
number_to_currency(price)
endContext
StackExchange Code Review Q#91174, answer score: 8
Revisions (0)
No revisions yet.