3
\$\begingroup\$

Just a little exercise to make me more affluent with strings and error handling, any improvements welcome!

#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <stdbool.h>
#include <math.h>

#define MAXSIZE 1024

void retreiveStringInput(char *str, size_t buffersize) 
{
    for (;;) {
        if (fgets(str, buffersize, stdin) != NULL) {
            str[strcspn(str, "\n")] = 0;
            return;
        }
        else
            printf("Touble allocating memory, Please try again...\n");
    }
}

bool strHasAllDigits(char *str)
{
    size_t strsize = strlen(str);
    if (str[0] != 0) {
        for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
            if (isdigit(str[strIndex]))
                continue;
            else
                return false;
        }
        return true;
    }
    else {
        printf("String is empty...\n");
        return false;
    }
}

int strToInt(char str[])
{
    size_t len = strlen(str);
    size_t power = len - 1;
    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
        convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
    }
    return convertedString;
}

int main()
{
    char number[MAXSIZE];
    int intNumber = 0;
    for (;;) {
        printf("ENTER A WHOLE NUMBER: ");
        retreiveStringInput(number, MAXSIZE);
        if (strHasAllDigits(number))
            intNumber = strToInt(number);
        else {
            printf("Number must be a whole number containing no characters, please try again...\n\n");
            continue;
        }
        printf("Your number is: %d\n", intNumber);
        break;
    }

    printf("Press any key to continue...");
    getchar();
}
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Nitpicks

void retreiveStringInput(char *str, size_t buffersize) 

Misspelled.

void retrieveStringInput(char *str, size_t buffersize) 

Not a critical mistake, since you used it consistently. Don't forget to change other uses when you fix it.

            printf("Touble allocating memory, Please try again...\n");

This suggests that there is trouble allocating memory, but this code doesn't allocate memory. It reads from input.

It also has a typo in "Trouble".

Keeping it simple

    size_t strsize = strlen(str);
    if (str[0] != 0) {
        for (size_t strIndex = 0; strIndex < strsize; ++strIndex) {
            if (isdigit(str[strIndex]))
                continue;
            else
                return false;
        }
        return true;
    }
    else {
        printf("String is empty...\n");
        return false;
    }

Consider

    if (*str == '\0') {
        printf("String is empty...\n");
        return false;
    }

    for (; *str != '\0'; ++str) {
        if (!isdigit(*str)) {
            return false;
        }
    }

    return true;

I find it easier to use positive if conditions with else statements. So the if is the positive case and the else is the negative case. In this situation, we don't need an else then, as we return in the if.

There is no point in a continue when there is no statement to skip. Just let it go. It will continue automatically. You don't need an else case.

You only used the length as part of the condition check in the loop. But we don't need it. We can just check for the end of the string directly. We know that the end of string marker is '\0', so we can just check for that.

It's true that '\0' is 0, but it is more readable to write it out in my opinion.

I changed str[0] to *str because I knew that I was going to be saying *str in the for loop anyway. Rather than use two different things that return the same value, I picked the one that worked in both cases. It would look kind of odd to repeatedly do str[0] and act as if the values would be different. That's more understandable with *str.

I don't like the single statement form of control structures. I always use the block form as being more robust against editing mistakes and a little easier to read.

Avoid unnecessary floating point operations

    size_t power = len - 1;
    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex, --power) {
        convertedString += ((int)(str[strIndex] - '0') * pow(10,power));
    }

Consider

    int convertedString = 0;
    for (size_t strIndex = 0; strIndex < len; ++strIndex) {
        convertedString *= 10;
        convertedString += (int)(str[strIndex] - '0');
    }

We don't need power anymore.

We don't convert to and from a floating point type. No danger of precision losses. Everything is an integer type. We probably don't even need the explicit cast to int. I haven't run the code, so I left it.

Simplify

        if (strHasAllDigits(number))
            intNumber = strToInt(number);
        else {
            printf("Number must be a whole number containing no characters, please try again...\n\n");
            continue;
        }
        printf("Your number is: %d\n", intNumber);
        break;

Consider

        if (strHasAllDigits(number)) {
            int intNumber = strToInt(number);
            printf("Your number is: %d\n", intNumber);
            break;
        }

        printf("Number must be a whole number containing no characters, please try again...\n\n");

Now we don't have to declare intNumber outside the loop. And we don't have to continue to skip the break.

\$\endgroup\$
0

Your Answer

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

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