7
\$\begingroup\$

I want to split an integer of type unsigned long long to its digits. Any comments and suggestions are always welcome.

#include <iostream>
#include <string>

#include <vector>

using namespace std;

vector<unsigned short> IntegerDigits(unsigned long long input)
{
    vector<unsigned short> output;
    const unsigned short n = log10(ULLONG_MAX);
    unsigned long long divisor = pow(10, n);
    bool leadingZero = true;

    for (int i = 0; i < n + 1; i++)
    {
        unsigned short digit = input / divisor % 10;
        if (!leadingZero || digit != 0)
        {
            output.push_back(digit);
            leadingZero = false;
        }
        divisor /= 10;
    }

    return output;
}

void main()
{
    vector<unsigned short> output = IntegerDigits(ULLONG_MAX);

    cout << ULLONG_MAX << ": [";
    for (auto y : output)
        cout << y << ", ";
    cout << "\b\b]";

    cout << ""<<endl;

}
\$\endgroup\$
  • 1
    \$\begingroup\$ short n = log10(ULLONG_MAX) will this be rounded up or down? Why? \$\endgroup\$ – greybeard Oct 31 at 6:33
  • \$\begingroup\$ @greybeard: Down. Because log10(ULLONG_MAX) + 1 represents "the max number of digits" for unsigned long long. \$\endgroup\$ – Money Sets You Free Oct 31 at 6:40
  • 1
    \$\begingroup\$ What if ULLONG_MAX was 2⁶⁶ (2³³×2³³)? \$\endgroup\$ – greybeard Oct 31 at 6:44
  • \$\begingroup\$ @greybeard the max number of digits becomes 20. What is the problem? \$\endgroup\$ – Money Sets You Free Oct 31 at 6:51
  • 2
    \$\begingroup\$ The problem is that a rounded up pow(10, n) may not be "losslessly" assignable to an unsigned long long. \$\endgroup\$ – greybeard Oct 31 at 7:54
11
\$\begingroup\$

Avoid mixing floating point and integer arithmetic

As mentioned by greybeard, there is a potential problem here:

const unsigned short n = log10(ULLONG_MAX);

ULLONG_MAX is larger than can be exactly represented by a double. This means the result might not be what you expect. The same goes for pow(10, n). While you can compensate for it, it is better to find a way to calculate the length of a number without using floating point math.

Keep it simple

Unless performance is a big concern, keep it simple. You don't have to know the number of digits up front if you push trailing digits to the front of the vector, like so:

vector<unsigned short> IntegerDigits(unsigned long long input)
{
    vector<unsigned short> output;

    while (input)
    {
        output.insert(output.begin(), input % 10);
        input /= 10;
    }

    // Handle input being equal to 0
    if (output.empty())
    {
        output.push_back(0);
    }

    return output;
}

Pushing to the front of a std::vector is less efficient, but on the other hand you don't need the double<->int conversions, and you don't need to handle the leading zeros inside the loop.

Avoid using std::endl

Prefer using "\n" over std::endl, the latter is equivalent to the former, but also forces a flush of the output, which can be bad for performance.

Avoid backspaces in the output

You used a neat trick to get rid of the last comma without having to have extra logic inside the for-loop in main(). However, consider that the output might not just be for human consumption, but is written to a file and/or is parsed by another program. In that case, the \b characters are probably unexpected and might cause problems.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Where is push_front defined? \$\endgroup\$ – Money Sets You Free Oct 31 at 8:30
  • \$\begingroup\$ I changed to stack instead of vector because push_front has not been defined in vector. :-) \$\endgroup\$ – Money Sets You Free Oct 31 at 8:51
  • \$\begingroup\$ Ehr oops, I meant insert(output.begin(), ...)! \$\endgroup\$ – G. Sliepen Oct 31 at 10:16
  • 1
    \$\begingroup\$ Better to just append and then reverse in-place? \$\endgroup\$ – Deduplicator Oct 31 at 19:43
  • \$\begingroup\$ Or use a std::deque. The are many ways one could do this, but since it's likely only a few digits will be stored, I don't think it's going to matter much. This is where I would just keep the simplest approach, and only change it when performance becomes a bottleneck and measurements have shown that this function is a big contributor. \$\endgroup\$ – G. Sliepen Oct 31 at 21:03
1
\$\begingroup\$

As @G. Sliepen said, I also want to reiterate, when performance isn't a problem, ensure to make the code as simple as possible.

A modified version using stack might be written like this

void separate_digits( stack<int> &s, long long int digits )
{
    while( digits != 0 ) 
    {
        s.push( digits % 10 );
        digits /= 10;
    }
}

Displaying the digit would just require you to pop the stack, which as we know takes constant time

void print_separated_digits( stack<int> &s ) 
{
    while( !s.empty( ) )
    {
        std::cout << s.top( ) << " ";
        s.pop( );
    }
}
| improve this answer | |
\$\endgroup\$

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.