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);
}
}
}
-
4\$\begingroup\$ This is also built into the streams. You just have to set the local so it understands your country specific way of doing it. stackoverflow.com/a/3479520/14065 \$\endgroup\$Loki Astari– Loki Astari2017年10月18日 17:17:39 +00:00Commented Oct 18, 2017 at 17:17
-
2\$\begingroup\$ And here: stackoverflow.com/a/7260003/14065 \$\endgroup\$Loki Astari– Loki Astari2017年10月18日 17:31:04 +00:00Commented Oct 18, 2017 at 17:31
4 Answers 4
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;
}
-
\$\begingroup\$ +1, Would be great to put static assert on it, to display a clear message. \$\endgroup\$Incomputable– Incomputable2017年10月18日 16:21:11 +00:00Commented Oct 18, 2017 at 16:21
-
\$\begingroup\$ Can you provide me a link on where to find some more information on what is going on with class T? I would like to understand how it chooses types and how to run type checks. Thank you in advance. \$\endgroup\$Nick Pavini– Nick Pavini2017年10月19日 04:20:14 +00:00Commented Oct 19, 2017 at 4:20
-
1\$\begingroup\$ @NickPavini en.cppreference.com/w/cpp/language/function_template \$\endgroup\$Artemy Vysotsky– Artemy Vysotsky2017年10月19日 06:12:59 +00:00Commented Oct 19, 2017 at 6:12
ArtemyVysotsky has provided you with a improved solution, so I am just going to give some general advice:
- 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. n
is of typelong long
, butmod
is only of typeint
. Sincemod
grows untiln % mod == n
, you are running into the risk of overflow here, somod
should also be of typelong long
. However, sincemod
should never be negative, it would be even better to have it be aunsigned long long
/- 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). - 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.
- Don't treat single characters as strings when treating them as
char
s would work fine. In particular, preferstd::cout << ... << ',';
overstd::cout << ... << ","
since this is (likely to be) faster and more efficient. - 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 astd::string
or taking anstd::ostream
as a parameter and output to that instead. - You don't actually need
return 0;
inmain
, the compiler is nice enough to add that automatically (Beware, however, as this only happens inmain
!).
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.
-
3\$\begingroup\$ Maybe to do magical things you need to be skilled. But to simply turn it on requires one command:
std::cout.imbue(std::locale(""));
Prefer not to use the specific local"de_DE"
. If you use the local""
it picks up the local specified on the host machine (in this modern day this is usually set correctly) and provides the correct output for human readability. If you want to generate transportable text output you should use the"C"
local. \$\endgroup\$Loki Astari– Loki Astari2017年10月18日 17:24:52 +00:00Commented Oct 18, 2017 at 17:24 -
2\$\begingroup\$ Also its not that many lines of code: stackoverflow.com/a/3479520/14065 \$\endgroup\$Loki Astari– Loki Astari2017年10月18日 17:28:47 +00:00Commented Oct 18, 2017 at 17:28
-
\$\begingroup\$ For simple examples on how to use facets see this question: codereview.stackexchange.com/a/71246/507 read to the bottom for links to lots of example usages. \$\endgroup\$Loki Astari– Loki Astari2017年10月18日 17:29:38 +00:00Commented Oct 18, 2017 at 17:29
-
\$\begingroup\$ @LokiAstari, thanks for the links, will probably post a question after I'll deal with midterms tomorrow. \$\endgroup\$Incomputable– Incomputable2017年10月18日 17:32:32 +00:00Commented Oct 18, 2017 at 17:32
-
\$\begingroup\$ If your platform supports it, you can explore grouping rules in shell: try
LC_NUMERIC=en_IN printf "%'d\n" 5000000
, for example. This gives50,00,000
as output. \$\endgroup\$Toby Speight– Toby Speight2017年10月18日 17:47:57 +00:00Commented Oct 18, 2017 at 17:47
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.`