Format an integer with thousands separator (recursive implementation)

Question

The function accepts a number n and displays it recursively with commas.

As well as a general code review, I'm interested in suggestions for alternatives to counting to tell when to print the commas. I tried if(mod%10000 == 0); that works on the first 3 numbers, but after that mod is always a multiple of 10000.

#include <iostream>
using namespace std;

void RecursiveCommas(long long n, int mod = 10, int i = 0);

int main()
{
    RecursiveCommas(123456789);
    return 0;
}

void RecursiveCommas(long long n, int mod, int i){
    if(n%mod==n){ //base case
        if(i%3 == 0 && i != 0)
            cout << (n%mod) / (mod/10) << ",";
        else
            cout << (n%mod) / (mod/10);
    }
    else{
        if(i%3 == 0 && i != 0){ //add commas
            RecursiveCommas(n, mod*10, i+1);
            cout << (n%mod) / (mod/10) << ",";
        }
        else { //no comma needed
            RecursiveCommas(n, mod*10, i+1);
            cout << (n%mod) / (mod/10);
        }
    }
}

Answer 1

Seems to me you overcomplicate the things.

IF you split your number into "beginning" and "last 3 digits" inside your RecursiveCommas you have to call it recursively on the "beginning" and then print the "last 3 digits". The only difference will be - that 1st chunk could be less than 3 digits and has no comma before it, all other chunks - following comma have to be printed with 0 filling.

Here is a sample implementation of the above approach (template is just to avoid selecting specific integer type - so I'm skipping the checks of the T intentionally):

#include <iostream>
#include <iomanip>

template<class T>
void RecursiveCommas(std::ostream& os, T n)
{
    T rest = n % 1000; //"last 3 digits"
    n /= 1000;         //"begining"

    if (n > 0) {
        RecursiveCommas(os, n); //printing "begining"

        //and last chunk
        os << ',' << std::setfill('0') << std::setw(3) << rest;
    }
    else
        os << rest; //first chunk of the number
}

int main()
{
    RecursiveCommas(std::cout, 123456789);
    std::cout << '\n';
    RecursiveCommas(std::cout, 1234);
    std::cout << '\n';
    RecursiveCommas(std::cout, 123);
    std::cout << '\n';
    RecursiveCommas(std::cout, 123456789012345);
    std::cout << '\n';

    return 0;
}

Answer 2

ArtemyVysotsky has provided you with a improved solution, so I am just going to give some general advice:

1. Do not use using namespace std; since it not only lowers readability of your code (things that belong to the standard library are no longer easily discernible) but also opens the door for subtle name resolution bugs.
2. n is of type long long, but mod is only of type int. Since mod grows until n % mod == n, you are running into the risk of overflow here, so mod should also be of type long long. However, since mod should never be negative, it would be even better to have it be a unsigned long long/
3. Keep your spacing consistent. For example, why do you sometimes leave space around == and other times don't? There is no right coding style that everyone agrees on, so you are free do choose any option that fits you, but you should at the very least stick to the style you choose throughout your code (also, keep in mind that most programmers consider more space better than less space, so leaving a space around operators is generally better received than not doing so).
4. Consider wrapping your recursive function in a helper function. Anyone who doesn't know about your code would assume that all three parameters your function takes can and should be passed by the caller when the last two arguments aren't actually supposed to take anything but their default value.
5. Don't treat single characters as strings when treating them as chars would work fine. In particular, prefer std::cout << ... << ','; over std::cout << ... << "," since this is (likely to be) faster and more efficient.
6. Having a function output do std::cout directly severely harms its uses. For example, what if I want to send the formatted number to a file? Currently, your function doesn't help much with that. There are several possible improvements here, for example building and returning a std::string or taking an std::ostream as a parameter and output to that instead.
7. You don't actually need return 0; in main, the compiler is nice enough to add that automatically (Beware, however, as this only happens in main!).

Answer 3

I believe the right thing to implement what you want is std::num_put. It is not hard to make simple stuff work, as Loki pointed out.

The usage would be like this:

std::vector<int> nums{/*some numbers*/};
std::cout.imbue(std::cout.getloc(), new coma_num_put);
std::copy(nums.begin(), nums.end(), 
          std::ostream_iterator<int>(std::cout, " "));

If nums containes {1000, 20043, ...}, the output will be as expected:

1,000 20,043 ...

What is better though, the standard library provides opportunity to install your own locale, which your system probably has.

For example, you can refer to the code on the cppreference page:

#include <iostream>
#include <locale>
#include <string>
#include <iterator>

int main()
{
    double n = 1234567.89;
    std::cout.imbue(std::locale("de_DE"));
    std::cout << "Direct conversion to string:\n"
              << std::to_string(n) << '\n'
              << "Output using a german locale:\n"
              << std::fixed << n << '\n'
              << "Output using an american locale:\n";
    // use the facet directly
    std::cout.imbue(std::locale("en_US.UTF-8"));
    auto& f = std::use_facet<std::num_put<char>>(std::cout.getloc());
    f.put(std::ostreambuf_iterator<char>(std::cout), std::cout, ' ', n);
    std::cout << '\n';
}

I am not author of the code though.

Imbuing the right locale will also bring other locale specific formatting options.

Answer 4

Firstly, I'm assuming that you're doing this as an exercise, and you don't want to use the Standard library facilities (which know whether to use ,, ., space or nothing to separate the digit groups, and understand which positions they go in even in locales such as India).

Don't using namespace std;

This is a bad habit to get into, and can cause surprise when names like begin and size are in the global namespace. Get used to using the namespace prefix (std is intentionally very short), or importing just the names you need into the smallest reasonable scope.

Think about types

The function accepts a long long, but it doesn't behave very well with negative numbers:

RecursiveCommas(-123456789ll);

-1-2-3,-4-5-6,-7-8-9

There are two ways to deal with this:

• void RecursiveCommas(unsigned long long n, unsigned int mod = 10, int i = 0);
  

• if (n < 0) {
      RecursiveCommas(-n, mod, int); // still a problem when 
      return;
  }
  

Note that -n may be undefined for some very negative values.

Also, mod needs to be as wide as n, otherwise the repeated mod*10 will overflow (this is the cause of the bug in the code, giving incorrect results in the more-significant digits (from around 10^10 on my platform, for example).

Consider working in thousands

Instead of dividing by ten, and keeping count of how many times you've divided by ten, divide by a thousand in each call:

#include <iostream>
#include <iomanip>

void RecursiveCommas(unsigned long long n)
{
    if (n < 1000) {
        std::cout << n;
    } else {
        RecursiveCommas(n/1000);
        std::cout << ',' << std::setw(3) << std::setfill('0') << n%1000;
    }
}

int main()
{
    RecursiveCommas(1234567890123456789ull);
    return 0;
}

1,234,567,890,123,456,789

(If you don't want to use << std::setw(3) << std::setfill('0'), you can write the digits individually).

Don't limit your output to std::cout

It's useful to be able to write to any std::ostream - in particular, std::ostringstream is often used when formatting for humans, and std::cerr may be more appropriate.`