Finding odd and even digits from an integer

Question

I was given a task:

Given any integer - write a program, which:

1. finds all digits of the integer

2. writes odd and even digits from the integer

3. finds the biggest even and smallest odd digit

4. writes the biggest possible number that can be formed by rearranging the odd digits

5. writes the smallest possible number that can be formed by rearranging the even digits

I have written this code, but I am wondering if I can somehow get it written shorter than this:

#include <iostream>
#include <cmath>
using namespace std;
int main()
{
    long a,s=0,s1=0;
    int b,n=0,n1=0,k=0;
    int d[10],nl[10];
    int c=0;
    int i=0,j=0;
    for (i=0; i<10; i++)
    {
        d[i]=0;
        nl[i]=0;
    }
    cout<<"Write a number: "<<endl;
    cin>>a;
    i=0;
    while(a!=0) {
        b=a%10;
        a=a/10;
        if(b%2!=0) {
            d[i]=b;
            i++;
        }
        else {
            nl[k]=b;
            k++;
        }
    }
    n=i;
    n1=k;
    cout<<n1<<" even numbers "<<n<<" odd numbers "<<endl;
    cout<<" Odd numbers not ranged array"<<endl;
    for(i=0; i<n; i++)
        cout<<d[i]<<" ";
    cout<<endl;
    cout<<" Even numbers not ranged array"<<endl;
    for(i=0; i<n1; i++)
        cout<<nl[i]<<" ";
    cout<<endl;
    c=0;
    for(i=0; i<n; i++)
    {
        for(j=0; j<n; j++)
        {
            if(d[i]>d[j])
            {
                c=d[i];
                d[i]=d[j];
                d[j]=c;
            }
        }
    }
    cout<<" Odd numbers ranged array"<<endl;
    for(i=0; i<n; i++)
        cout<<d[i]<<" ";
    cout<<endl;
    cout<<"Min odd number "<<d[n-1]<<endl;
    c=0;
    for(i=0; i<n1; i++)
    {
        for(j=0; j<n1; j++)
        {
            if(nl[i]<nl[j])
            {
                c=nl[i];
                nl[i]=nl[j];
                nl[j]=c;
            }
        }
    }
    cout<<" Even numbers ranged array"<<endl;
    for(i=0; i<n1; i++)
        cout<<nl[i]<<" ";
    cout<<endl;
    cout<<"Max even number "<<nl[k-1]<<endl;
    for(i=0; i<n; i++)
    {
        s=s*10+d[i];
    }
    for(i=0; i<n1; i++)
        s1=s1*10+nl[i];
    cout<<"Max odd digits' number "<<s<<endl;
    cout<<"Min odd digits' number "<<s1<<endl;
    return 0;
}

Answer 1

• Try to avoid using using namespace std.

• <cmath> is not utilized anywhere, so just remove it unless you end up adding associated code.

• Avoid giving variables single-letter names (unless they're simple loop counters, which is okay to do). Although it may save some typing, it will instead make them appear meaningless as there is no context. Comments can be added, but they're not a good substitute for actual variable names. If a variable needs a comment, chances are it hasn't been given a good name.

• To help increase readability and maintainability:

  • Have one variable per line. This also helps in case comments need to be added.

    long a;
    long s=0;
    long s1=0;
    

  • Separate operators and operands with whitespace so that the two can be identified.

    long a;
    long s = 0;
    long s1 = 0;
    

  • Separate code lines based on purpose, also so that readers can focus on certain parts.

    cout<<" Odd numbers ranged array"<<endl;
    
    for(i=0; i<n; i++)
        cout<<d[i]<<" ";
    
    cout<<endl;
    

  • Keep variables as close in scope as possible instead of listed together. This could also help determine if a variable is no longer being used.

    cout << "Write a number: " << endl;
    long a; // move this here
    cin >> a;
    

• std::endl also flushes the stream, which is slower. If you want just a newline, use "\n":

  cout << "Write a number: \n";
  

• This:

  c=nl[i];
  nl[i]=nl[j];
  nl[j]=c;
  

  can be done more concisely with std::swap:

  std::swap(n1[i], n1[j]);
  

• Consider making this program more modular (using functions). Keeping everything in main() could make it hard for yourself and others to read and maintain the code. It is also not clear, just from looking at it, that this program is written to fulfill those five tasks. It could just be one task if the reader weren't otherwise aware of them.

  This can be done any way you'd like, but the most likely solution could be to have one function for each of those tasks (while giving them appropriate names). They could then be called from main(), and you could also get the user input from main() (this could, for instance, allow you to terminate the program right away if invalid input is given).

Answer 2

towards the end of your code you have :

for(i=0; i<n; i++)
{
    s=s*10+d[i];
}
for(i=0; i<n1; i++)
    s1=s1*10+nl[i];

these are two different coding styles that can confuse a programmer in a large set of code, you should have written the second the same as the first, or written them all using the same coding style. I prefer the first myself.

for(i=0; i<n1; i++)
{
    s1=s1*10+n1[i];
}

this defines when the for loop is over and makes it so that it is more readable, I think.

---

this doesn't make the code shorter, but does make it more readable in my opinion.

Answer 3

Before you aim for brevity, you should focus on clarity. Your function is obfuscated enough, without trying to shorten it. The code will become somewhat shorter as you extract common snippets into functions. The key is not to aim for the minimum number of characters in the source code, but to limit each function to a reasonable number of lines.

STL vectors are easier to work with than arrays. Since you did not use vectors, I'll assume that you are deliberately avoiding them as a beginner.

Correctness

• Handling of zero: I would consider zero to be a number consisting of one even digit. You treat it as a zero-digit number. Worse, you say that the smallest odd digit is 0 and the largest number that can be formed using the odd digits is also 0.
• Handling of negative integers: If the input is negative, I think that the digit analysis should act on the absolute value of the number. Instead, your program seems to have a concept of "negative digits".

Maintainability

• Problem decomposition: You should decompose the problem into functions; otherwise you end up with a god-awful main() function that contains everything. I realize that this problem doesn't naturally lend itself to decomposition due to its eclectic nature, but still, it is possible. I'll discuss this further below.
• Variable naming: I'll just echo everyone else's complaints.
• Variable declarations: Declare your variables as close as possible to the point of use. There is no reason to declare all variables at the beginning of the function in C++. (The same goes for C since the C99 standard.)
• Magic number: Your arrays have size 10, a "magic number". At the least, you should explain in a comment that long can be up to 10 digits in your C++ environment. (This assumption is not portable!) The correct approach is to use std::numeric_limits<long>::digits10 (which is defined in #include <limits>).

Data Types

Array indices should be of type size_t.

I suggest treating base-10 digits as unsigned short. Furthermore, typedef unsigned short digit would be handy.

You can templatize the code to handle even larger inputs, such as long long. I'll use template <typename T> below, but you could just continue to use long.

Decomposition

The most obvious code that you can immediately extract is your sorting routine. If you used STL std::vector to store the digits, then it would be easiest to call std::sort(). If you use plain arrays, then you could call qsort() (in #include <cstdlib>) with one of the following comparators:

static int ascending_digits(const void *pa, const void *pb) {
    digit const &a = *static_cast<digit const *>(pa);
    digit const &b = *static_cast<digit const *>(pb);
    return (a < b) ? -1 :
           (a > b) ? +1 : 0;
}

static int descending_digits(const void *pa, const void *pb) {
    return -ascending_digits(pa, pb);
}

Other than that, I would recommend that the problem be decomposed into the following functions.

/**
 * Extracts the base-10 digits of number into an array.  The caller must
 * provide an array that is large enough for any value of type T, i.e.
 * it should have a size of at least std::numeric_limits<T>::digits10
 * (which is defined in #include <limits>).
 *
 * If number is negative, its absolute value is considered.
 *
 * The digits array will be filled, least-significant digit first.
 * Returns the actual number of digits in the number.
 */
template <typename T>
static size_t number_to_digits(T number, digit digits[]);

/**
 * Interprets an array of base-10 digits (least-significant digit first)
 * as a number.
 */
template <typename T>
static T digits_to_number(const digit digits[], size_t ndigits);

/**
 * Given an array of digits, copies just the odd digits (when parity=1)
 * or just the even digits (when parity=0).  The size of both the input
 * digits array and the output filtered array is specified by ndigits.
 *
 * Returns the number of digits copied into the filtered array.
 */
static size_t filter_digits(bool parity, const digit digits[], digit filtered[], size_t ndigits);

/**
 * Prints the label, followed by '\n', followed by n space-delimited digits,
 * followed by std::endl.
 *
 * Returns out for chaining convenience.
 */
static std::ostream &output_digits(std::ostream &out, const char *label, const digit digits[], size_t n);

/**
 * Prints the analysis of the number.
 */    
template <typename T>
void analyze(T number, std::ostream &out);

int main() {
    long long number;
    std::cout << "Enter a number: " << std::endl;
    std::cin >> number;
    analyze(number, std::cout);
}