Provide 2 Cents for A Simple Program

[mpark4656]

I wrote a simple program to find individual digits of a given number and also find the sum of all digits.

For example, if user input is 1234
It is supposed to print 1 2 3 4 and the sum of 10

Same with negative numbers like -1234
1 2 3 4
10

This code will be a member function of a class eventually, but for testing and debugging purpose, I made this program.

I just want some feedback on this if possible. As far as I know, it works. But I am wondering how it could've been more efficient. I want to know how I can improve my coding skills and/or improve the algorithm used in this program.

Thanks in advance!

#include <iostream>
#include <cmath>
#include <new>

int main ()
{
    // Saves the number of digits. Initialized to 1 since all numbers will have at least 1 digit
    int numberOfDigits = 1;

    // Saves each digit in the given num
    int digit = 0;

    // Saves the number input
    int num = 0;

    // Pointer Variable for digit
    int *digitPtr = nullptr;

    // Prompt for num input
    std::cout << "Enter the number: " << std::flush;

    std::cin >> num;

    // Make a copy of num to prevent accidential change
    int copyNum = std::abs ( num );

    // Count the number of digits
    // While the number divided by 10 is not 0, the loop will keep running, incrementing numberOfDigits each time
    while ( copyNum / 10 != 0 )
    {
        // Increment numberOfDigits
        numberOfDigits ++;

        // Divide the number by 10
        copyNum = copyNum / 10;
    }

    // This is a debugging output to decide whether the number of digits were counted correctly. Comment it out when finished
    //std::cerr << "The digit count is " << numberOfDigits << std::endl;

    // Reset the copyNum variable since it was change while counting digits
    copyNum = std::abs ( num );

    // Reference digit variable
    digitPtr = &digit;

    // Now that we have the number of digits, use pointer to create array of digit variable
    digitPtr = new int [numberOfDigits];


    // copy numberOfDigits to another variable to prevent accidental changes to the actual numberOfDigits
    int copyNumberOfDigits = numberOfDigits - 1;

    // Save each digit to the dynamic array
    for ( ; copyNumberOfDigits >= 0; copyNumberOfDigits -- )
    {
        digitPtr[copyNumberOfDigits] = copyNum % 10;

        copyNum = copyNum / 10;
    }

    // Initialize a variable sum to calculate the sum of all digits
    int sum = 0;

    // For Loop for adding all digits
    for ( int i = 0; i < numberOfDigits; i ++ )
        sum = sum + digitPtr[i];

    // Output the result
    std::cout << "The digits are ";

    for ( int i = 0; i < numberOfDigits; i ++ )
        std::cout << digitPtr[i] << " ";

    std::cout << std::endl << "The sum is " << sum;

    // Free up the allocated memory (Since this is the end of program anyways, it's not really needed, but still good practice)
    delete [] digitPtr;

    // Return 0
    return 0;
}

[booradley60]

A lot of these comments are unnecessary, and actually make the code more difficult to read. Use good variable names and function names to take the place of comments. For example,

// Saves the number input
    int num = 0;

could be better and more concise just by removing the comment and naming the variable inputNum. Comments like this

// Return 0
    return 0;

add nothing.

Algorithmically, if you use a std::vector to hold your digits you can put them in the collection and calculate the sum in the same loop as you go. This removes manual memory management from your code, too.

#include <iostream>
#include <vector>
#include <cmath>

int main ()
{
    // container to hold individual digits
    std::vector<int> digits;

    // Prompt for num input
    int inputNum;
    std::cout << "Enter the number: " << std::flush;
    std::cin >> inputNum;

    // Make a working copy of input that's positive
    int copyNum = std::abs ( inputNum );
    int sumOfDigits = 0;

    do
    {
        int nextDigit = copyNum % 10; //get the next digit
        digits.push_back(nextDigit);  //put it in the collection
        sumOfDigits += nextDigit;     //keep running sum of digits
        copyNum /= 10;                //integer-divide it out of the copy
    } while ( copyNum != 0 );

    // Output the result most-significant digit first
    // Use reverse iterator, since digits were inserted least-significant digit first
    std::cout << "The digits are ";
    for ( std::vector<int>::const_reverse_iterator iter = digits.crbegin();
          iter != digits.crend();
          ++iter)
    {
        std::cout << (*iter) << " ";
    }

    std::cout << std::endl << "The sum is " << sumOfDigits;

    return 0;
}

EDIT: Changed while to do...while to handle case where input is 0.

[Chervil]

You asked for comments, so here's a few thoughts. In the original code, a number of variables are declared right at the start. Though some programming languages may force us to do that, in C++ it is usually preferable to declare variables as close as possible to where they are used, and to limit their scope to the part of the code where they are needed.

I'd suggest putting comments down the right hand side - and keep them brief, rather than having them interspersed with the code.

This is the original program code, just rearranged a little. Minor tweaks in a couple of places. Actually I don't think new/delete is really called for here, but I left it in.

#include <iostream>
#include <cmath>

int main ()
{
   /**********************************************
    *  Find individual digits of a given number  * 
    *  and also find the Sum of all digits.      *
    **********************************************/
    
    int num;
    std::cout << "Enter the number: "          // get the number
              << std::flush;
    std::cin >> num;
        
    int copyNum = std::abs( num );             // Make a copy of num 

    int numberOfDigits = 1;                    // Count the number of digits
        
    while ( copyNum /= 10 )
        numberOfDigits ++;

    copyNum = std::abs ( num );                // Reset copyNum 

    int *digitPtr = new int [numberOfDigits];  // create array to store digits

    for (int i = numberOfDigits; --i >= 0; )   // Save each digit to the array
    {
        digitPtr[i] = copyNum % 10;
        copyNum = copyNum / 10;
    }

    int sum = 0;                               // Get sum of all digits
    for ( int i = 0; i < numberOfDigits; i++ )
        sum = sum + digitPtr[i];

    std::cout << "The digits are ";            // Output the digits

    for ( int i = 0; i < numberOfDigits; i++ )
        std::cout << digitPtr[i] << " ";

    std::cout << "\nThe sum is " << sum;       // and the sum

    delete [] digitPtr;                        // Free the allocated memory 

    return 0;
}

[mpark4656]

Thank you both for the comments.

@booradley60

Using std::vector is definitely more efficient! I don't know why I did not even think of that.

You are right about the way I use comments. I do realize that I have unnecessary comments. I will work on that



@Chervil

I will make a habit of placing comments on the right side like you did. It does look more clean. And concise comments look much better