3
\$\begingroup\$

As some of you might know I've been working my way through the C Programming Language (2nd).

I recently finished a rough implementation of the itoa exercise (chapter 3). It's important to note that we haven't been introduced to any advanced subjects like pointers, dynamic arrays etc yet.

The exercise is to create a string representation of a number with a left padding of spaces depending on a parameter width. In my case the number is 1030 and the width is 10, meaning the representation is ______1030 (replace _ with ).

I also have decided to use as few external packages as possible so that I can learn more about C rather than knowing how to implement packages. This might be against some best practices so I thought it would be best to mention this before you get to my implementation.

I would like to get general feedback, constructive criticism on the solution and especially help with identifying any particular syntactical errors or potential bottlenecks. In this implementation I've decided to do one static test rather than allowing the user to input data.

I am especially interested in how I could possibly make the loop a bit more effective as I really don't like that I need to use a second statement to divide number.

You can find my solution on Gist if you wish to have it formatted, or below:

/*
    itoa.c (i, s, w)
    @param i: integer
    @param s: char []
    @param w: width
    @returns a string representation of i with any excess (left) padding
            in blankspaces dependant on the width w.
*/



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

#define MAXLINE 100

unsigned numDigits(const unsigned n) {
    if (n < 10) return 1;
    return 1 + numDigits(n / 10);
}

void itoa(int number, char string_out [], int width) {

    int i = width;
    do {
        string_out[i] = (number > 0) ? number % 10 + '0' : ' ';
        number /= 10;
    } while ((i--) != 0);
    string_out[width + 1] = '\0';
}

int main () {

    char str [MAXLINE];
    if (numDigits(1030) > 10) {
        printf("Sorry, the number you provided was longer than 10 characters\n");
        return 1;
    }
    itoa(1030, str, 10);

    printf("Before: %d\nAfter: %s\n", 1030, str);

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ What should itoa(12345, string_out, 3) do? Make "12345"? How about itoa(12345, string_out, 300)? or itoa(-12345, string_out, 10), itoa(12345, string_out, -10)? \$\endgroup\$
    – chux
    Commented May 31, 2017 at 15:38

2 Answers 2

2
\$\begingroup\$

The itoa function pretty much nails the solution, I can't think of a way to remove the division by 10.

Better error Checking
The itoa in the code function can't produce negative numbers so it would be better to check for negative numbers in the calling function.

Performance Consideration
Consider what the C programming language is used for, the primary use is as a high level assembly language for either writing operating systems or embedded code, therefore performance is very important.

Where the real performance issue in this code is numDigits(). C does allow recursion, and the are times when recursion provides the best solution, but this problem isn't one of them. There is a very simple test that can be performed in one line of code. If the number greater than 9999999999 then it is more than 10 digits.

String Size
The size of the character array is not a power of 2. When using C it is best to make character arrays a power of 2, 4 or 8 depending on the word size. Making a character array of any other size may lead to memory alignment problems if the character array is used in a struct or in more complex code.

\$\endgroup\$
4
  • \$\begingroup\$ Great feedback, man. Thanks for taking some time to read through my solution and writing this up. Do you have any advice on how to solve the numDigits() function without recursion? Ie. reducing the risk of overflow? \$\endgroup\$
    – geostocker
    Commented May 27, 2017 at 13:38
  • 1
    \$\begingroup\$ @geostocker No function is needed to replace the recursion, it is a simple if statement if (number > 9999999999) error message. \$\endgroup\$
    – pacmaninbw
    Commented May 27, 2017 at 13:59
  • \$\begingroup\$ Keep in mind that the limit of this entire program is INT_MAX it it continues to use int, and INT_LONG if it switches to long. See limits.h. \$\endgroup\$
    – pacmaninbw
    Commented May 27, 2017 at 14:16
  • 1
    \$\begingroup\$ "When using C it is best to make character arrays a power of 2, 4 or 8 depending on the word size." is a minor performance idea that depends greatly on platform and coding needs. Insuring width is not too big (a test that is missing) and overflowing the buffer is more valuable. \$\endgroup\$
    – chux
    Commented May 31, 2017 at 15:31
0
\$\begingroup\$

I think it's quite well done, but I think this would look better if it was in 2 lines, I mean return below if if (n < 10) return 1;. About number /= 10 I think you can end your loop when you finish writing number. And then just fill rest of the array with blanks. I mean use another loop for writing blanks, it will save you the time of doing unnecessary number /= 10 and checking value of number. You can do this in a loop if you don't want to use other functions, or use memset.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.