5
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You mean better, more object-oriented and in line with best practices, or just shorter? Right place for the former, codegolf.stackexchange.com for the latter :) \$\endgroup\$ – Mathieu Guindon Dec 2 '13 at 13:58
  • 1
    \$\begingroup\$ I meant everything You mentioned. \$\endgroup\$ – EllA Dec 2 '13 at 14:02
  • \$\begingroup\$ An algorithm/optimisation review then! +1 from me \$\endgroup\$ – Mathieu Guindon Dec 2 '13 at 14:07
  • \$\begingroup\$ Are you avoiding STL on purpose? \$\endgroup\$ – 200_success Dec 2 '13 at 23:22
  • 2
    \$\begingroup\$ You could not have made the program harder to read by naming all your variables a1,a2,a3,a4,a5,a6. Come on write code with meaningful names. \$\endgroup\$ – Martin York Dec 3 '13 at 0:03
9
\$\begingroup\$
  • 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).

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ It also allows you to easily add more to the body if needed. \$\endgroup\$ – Jamal Dec 3 '13 at 0:44
  • 2
    \$\begingroup\$ It also prevents errors (using the braces). Badlly written macro functions can cause the the loop to be applied only to the first statement in the macro. So this is also a defensive style that prevents errors. I will always use {} were there is a nested block statement for exactly this purpose and reject code from code review unless they also follow this convention. After the first time you find a bug caused by this problem you will never leave them out either :-) (because it is a bug that is really hard to find). \$\endgroup\$ – Martin York Dec 3 '13 at 17:11
4
\$\begingroup\$

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 .

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);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Agree with everything apart from size_t and unsigned usage. Until very recently I also applied the above rule in my code (because I got it from the behavior of the standard library). But while at "Going Native" all the big guys in the C++ community said that it was a bad idea. You should use signed types for everything (apart for ints that you are using to hold bit flags). They basically said that the got it wrong when designing the standard library. \$\endgroup\$ – Martin York Dec 3 '13 at 17:05
  • 1
    \$\begingroup\$ See this video of the discussion. The general discussion on int starts at 41:05 The specific bit about unsigned starts at 42:54 Herb starts his apology at 44:25 and wraps up at 46:00. The basic argument is that mixing signed/unsigned introduces errors. But the two problems it (using unsigned) tries to solve don't really exist. Example: setSize(unsigned int s) Now call it like this: setSize(-1); Works perfectly well but probably does not do what you want. \$\endgroup\$ – Martin York Dec 3 '13 at 17:06
  • \$\begingroup\$ Sorry: Copy and pasted the comment from another thread (but I think it is important enough to repeat). \$\endgroup\$ – Martin York Dec 3 '13 at 17:07
  • \$\begingroup\$ @LokiAstari: I'll keep that right here. \$\endgroup\$ – Jamal Dec 4 '13 at 1:43

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.