3
\$\begingroup\$

Using JSX syntax I wrote a function that takes either a float or an integer as a parameter and returns a formatted string. I'd like any feedback for refactoring the code.

const moneyFormat = (float) => {
  let answer = [];
  let firstHalf;
  let secondHalf;

  // check if whole number
  if (float % 1 === 0) {
    float = Math.trunc(float);
    firstHalf = float.toString();
    secondHalf = '.00';
  } else {
    float = float.toFixed(2);
    const splitNumArr = float.split('.');
    firstHalf = splitNumArr[0];
    secondHalf = '.' + splitNumArr[1];
  }

  let everyThree = 0;
  // push chars into answer array
  for (let i = firstHalf.length - 1; i >= 0; i--) {
    answer.push(firstHalf[i]);
    everyThree += 1;
    // insert space for every three chars 
    if (everyThree % 3 === 0) {
      answer.push(' ');
    }
  }
  // reverse array and add secondHalf
  answer = answer.reverse().join('') + secondHalf;
  return answer;
}

console.log(moneyFormat(2310000.159897)); // '2 310 000.16'
console.log(moneyFormat(1600)); // '1 600.00'

\$\endgroup\$
  • 1
    \$\begingroup\$ I assume you (like me) are a beginner and are making your own code for the things you learn while doing so :) If you were doing this for a production server you would, of course, Google to find stackoverflow.com/questions/149055/…. Just chucking this link here for anyone coming from a search engine looking for a money formatter. \$\endgroup\$ – DarkMatterMatt Dec 23 '17 at 6:41
4
\$\begingroup\$
  • Bad variable names. dollars and cents would be more appropriate that firsthalf and secondhalf... I mean, c'mon, these variables don't represent "halfs" of anything. float isn't very descriptive either. Perhaps rawvalue would be better, and maybe result_buffer would be better than answer.
  • The entire code for whole numbers is unnecessary. When you run toFixed() on a whole number it becomes a float (string) anyway.
  • Rather than creating a new variable and arbitrarily swapping between let and const, why not just reassign float (or in my version, raw_value). You won't need it anymore after that anyway.
  • You might wanna consider casting it to a float in case a string is passed to the function, in which case your toFixed would fail.
  • It doesn't make a hue difference whether you declare your variables at the top or as needed when you're working on a small function, but be consistent. And if you do declare them all at the top, I would suggest doing it in a single statement for a small performance boost. In this case, I would personally just declare them as you go to keep the function concise.
  • When you're doing a backwards loop like that you don't need all three statements in the for loop. You could just do for (let i = firstHalf.length; i--;). That said, looping backwards and then reversing (or looping at all for that matter) makes for smelly code. You can just use a little regular expression to chunk the chars up: /.{1,3}/g
  • Don't reassign a variable just to return it, just return it. It's less work for the computer that way.

const moneyFormat = (raw_value) => {
  raw_value = parseFloat(raw_value).toFixed(2).split('.');
  let dollars = raw_value[0],
    cents = '.' + raw_value[1],
    format_buffer = dollars
      .split('').reverse().join('') // reverse it
      .match(/.{1,3}/g) // chunk it
      .map(c=>c.split('').reverse().join(''))
      .reverse(); // reverse it again
  return format_buffer.join(' ') + cents;
}

console.log(moneyFormat(2310000.159897)); // '2 310 000.16'
console.log(moneyFormat(1600)); // '1 600.00'
console.log(moneyFormat("1600"));
console.log(moneyFormat("234123.2134123"));

\$\endgroup\$
  • 1
    \$\begingroup\$ Uh, when I run your code snippet your numbers are reversed in each group of three, e.g. moneyFormat(1600) -> 1 006.00 \$\endgroup\$ – DarkMatterMatt Dec 23 '17 at 6:29
  • 1
    \$\begingroup\$ @DarkMatterMatt - lolwut.. it's 2am why am i answering things on stackexchange... fixed it but it's a little more complicated than i would have liked.. hopefully someone else has a better idea \$\endgroup\$ – I wrestled a bear once. Dec 23 '17 at 6:36
  • 1
    \$\begingroup\$ It might be worth defining const reverse = str => str.split('').reverse().join('') \$\endgroup\$ – Gerrit0 Dec 26 '17 at 22:28

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.