2
\$\begingroup\$

Problem: Given an integer between 1 and 32767 print the individual digits with 2 spaces between them.

I am using C How To Program but this is not homework. The book has not gotten to arrays or anything more complicated than looping structures and function. I try to avoid using those structures and concepts that have not been introduced yet. I get the digits to separate and output. I was wondering if there is a better way to do this using the tools I have.

My Code:

//-----------------------------------------------------------------------------
// Program: EX5_22.c - Separating Digits
// Programmer:  Joseph Cunningham
// Class: CsC_20 - c
// Date: 2/16/19
//
// This program will prompt the user for a number from 1 - 32767.  It will then
// output that number separated into its constituent digits with 2 spaces
// between each digit
//-----------------------------------------------------------------------------

#include<stdio.h>
#include<math.h>

int getNumLength(int number);
void separate(int number, int power); // Fucntion prototype

int main (void)
{
  // Variable declaration

  int number; // User entered number to digitize
  int power;  // Power to generate divisor

  // Prompt user for number

  printf("Please input a number (1-32767): ");
  scanf("%d", &number);

  // Get the power for the divisor

  power = getNumLength(number);

  // Output digits

  separate(number, power);

  return 0;
}

//-----------------------------------------------------------------------------
// Function: getPower(number) - counts the number of digits in the number to
// generate a power
// Input:  int number - the user entered number
// Output: none
// Return value - length of thr number
//-----------------------------------------------------------------------------

int getNumLength(int number)
{
  int length; // the length of number

  while (number != 0)
  {
    number = number / 10;
    length++;
  }

  return length;
}

//-----------------------------------------------------------------------------
// Function: separate(number) - separates and displays an integer between 1
// -32767 into its digits
// Input: int number - intger betwenn 1 - 32767, int power - power for divisor
// Output: numbers digits separated by 2 spaces
// Return value: none
//-----------------------------------------------------------------------------

void separate(int number, int power)
{
  int divisor; // the divisor from power
  int i;  // loop counter

  for(i = power - 1; i >=0; i--)
  {
    divisor = (int)pow(10, i);
    printf("%d  ", number / divisor);
    number = number % divisor;
  }
}
\$\endgroup\$
  • \$\begingroup\$ Looks interesting (Is case more complicated than looping?). But, as the code is known not to work as intended, it is off topic here. \$\endgroup\$ – greybeard Feb 18 '19 at 0:09
  • \$\begingroup\$ I'll change the code above to one that works as intended but may need optimizing? \$\endgroup\$ – Jay Feb 18 '19 at 0:15
  • \$\begingroup\$ Sounds promising: Welcome to Code Review! Even if your post gets closed before you're done, it will be bound to be re-opened once fixed. (Closing would be for the better: it is an indication that the question is not in a state to be answered.) Note that you should not alter your code a considerable time after posting, and are forbidden to so in a way that invalidates answers. \$\endgroup\$ – greybeard Feb 18 '19 at 0:23
  • 2
    \$\begingroup\$ Can I take it down and try again later? \$\endgroup\$ – Jay Feb 18 '19 at 0:34
  • \$\begingroup\$ Yes, of course. Removing content that is useful for others is frowned upon, but possible, too. \$\endgroup\$ – greybeard Feb 18 '19 at 0:36
4
\$\begingroup\$
  // Variable declaration

  int number; // User entered number to digitize
  int power;  // Power to generate divisor

So // is a C++ comment that is only available in newer versions of C.

Declaring variables at the beginning of the block is an old C standard that you don't need in newer versions of C. The modern standard (in C and most other languages) is to put variable declarations as close to first use as possible. For number, you're pretty much there. For power, it could be declared with the initialization.

    /* Power to generate divisor */
    int power = getNumLength(number);

I put the comment on a separate line so as to be more readable and to avoid side scroll (not so much here as in other examples).

I would have probably have written that

    int digit_count = count_digits(number);

That's clearer in my opinion and does not require an explanatory comment. We're counting the number of digits and storing the result in a variable. We'll know what the variable is later, because we call it digit_count.

I prefer snake_case, particularly in C. It relies less on the reader (who may not be a native English speaker) being able to recognize capital letters. That said, camelCase is quite common. Which to use is up to you so long as you do so consistently.

I also changed to a four column indent. That is more common than two column in code. The only place that I'd recommend two column is in markup languages like HTML and XML. They don't have methods, so their indent increases with their complexity. With code, if you are indenting so much that you are running out of space on the screen, that may be a sign that you should push code into separate functions or methods.

An alternate approach

  int divisor; // the divisor from power
  int i;  // loop counter

  for(i = power - 1; i >=0; i--)
  {
    divisor = (int)pow(10, i);
    printf("%d  ", number / divisor);
    number = number % divisor;
  }

You are using the rather expensive pow function in each iteration of your for loop.

Consider

    for (int divisor = buildInitialDivisor(number); divisor > 0; divisor /= BASE) {
        printf("%01d  ", number / divisor);
        number %= divisor;
    }

This declares the loop variable as part of the loop declaration.

This uses a constant BASE instead of the magic number 10. This improves readability and makes the program easier to modify.

I changed from %d to %01d. Now if you wanted to display, say, two digits at a time, you could change that to %02d and change BASE to 100. It should zero pad the number so as to always print two digits. Of course, it would print a single leading digit as two digits too. That might be undesirable. You might consider how you could fix that.

The %= is just a shorter syntax. Your original line and the revised one will do the exact same thing. Assignment operators in C.

This gets rid of pow and replaces it with a different function and then divides on each iteration.

This gets rid of your i variable which only tracked the number of iterations. We can do that directly.

I prefer to always put the curly brackets on the same line as the code structure. Either form is fine so long as you are consistent. But you'll see both as you go, so might as well start recognizing them now.

This also requires

const int BASE = 10;

and

int buildInitialDivisor(int number) {
    int divisor = 1;
    while (number / BASE >= divisor) {
        divisor *= BASE;
    }

    return divisor;
}

But it gets rid of your getNumLength function.

Recursion

You may not have gotten there yet, but when you get to recursion, you might try this problem again. Recursion often helps when you need to reverse the natural order of output. This is because you can display while returning from the recursion. So rather than building the largest divisor first, you can incrementally increase the size of the divisor (or better yet, decrease the number, always using the same divisor).

You also might try this again after learning sprintf and character arrays.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you! This is a lot to digest and look up. I have vacillated between snake_case and camelCase over time and was never quit sure which one was better. Recursion is also in this chapter but at the end so I am pretty sure that there is an exercise revisiting this exercise but to make use of recursion. \$\endgroup\$ – Jay Feb 18 '19 at 6:12
  • \$\begingroup\$ "in newer versions of C" – in particular, any C compiler that implements C99 (from the year 1999) has it, so it's a safe bet \$\endgroup\$ – Roland Illig Feb 18 '19 at 7:15
  • \$\begingroup\$ Recursion should be avoided for most purposes and IMO should not be taught to anyone, least of all beginners. It is very rare to encounter trouble-free, correctly optimized recursion in any C application. Buggy, unreadable, dangerous recursion however, is a common source of problems. \$\endgroup\$ – Lundin Feb 18 '19 at 15:17
3
\$\begingroup\$

Overall the code looks good, but old style C. Standard C allows variable declarations inside function bodies, and also inside for loops:

for(int i=0; i<n; i++)

This has been in the C standard for 20 years and a common non-standard extension since 1990. So you might want to question why you are writing programs in such old-fashioned ways. If your teacher/book says "it's ANSI-C" then note that // comments aren't allowed in that old standard either.

Your function documentation comments don't correspond with the actual code.


The main issue is actually the use of the pow() function, which in turn requires the whole floating point library. Some compilers might be able to optimize it at compile-time, but I wouldn't count on it. On some systems like low-end microcontrollers, floating point isn't even an option.

So how to write this without access to pow() but just plain integers? The advantage of pow is that you can iterate digit per digit, starting at 10^n and go down towards 10^0. Without that option, you would rather iterate from least significant digit and upwards. And this in turn complicates printing, because we want to print the most significant digit first.

A naive implementation (doesn't support negative numbers etc) without pow might look like this:

#include <stdio.h>

int get_digits (int n)
{
  int divisor=10000;
  int length;
  for(length=5; length>0; length--)
  {
    if(n/divisor)
    {
      break;
    }
    divisor/=10;
  }
  return length;
}

void print_with_spaces (const char* str)
{
  while(*str != '\0')
  {
    printf("%c  ", *str);
    str++;
  }
}

int main (void)
{
  int input = 12345;
  char output[5+1] = "0";
  int digits = get_digits(input);

  for(int i=0; i<digits && input!=0; i++)
  {
    if((input % 10) != 0)
    {
      output[digits-1-i] = (input%10) + '0';
    }
    input /= 10;
  }

  print_with_spaces (output);
}

Alternatively you can implement an integer version of pow yourself.

|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

I see some things that may help you improve your program. Here are a few things not mentioned by the other reviews.

Fix the bug

There is a problem with getNumLength because although it carefully counts digits, the value of length is never initialized. If you want it to be zero, you need to set it to zero.

Sanitize user input

It's possible for the user to enter a non-numeric value or zero or a negative number. Does your program correctly handle those cases? Generally, a robust program checks thoroughly for such problems and handles them in a rational way.

Consider alternative approaches

Note that the getNumLength and separate functions both do successive divisions of the same number. A more efficient approach would be to iterate through just once, calculating each digit, starting from the least significant digit and storing them in the appropriate structure for later printing. One approach using a fixed size string and pointers:

void print_digits(int number) {
    char answer[14] = "0  0  0  0  0";
    char *ptr = &answer[12];
    if (number) {
        while (number) {
            (*ptr) += number % 10;
            number /= 10;
            ptr -= 3;
        }
        ptr += 3;
    }
    puts(ptr);
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so it is not required to put return 0; explicitly at the end of main. I prefer to omit it; others don't. In any case, if you encounter that you'll know what it means.

|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.