10
\$\begingroup\$

Attempted to use strtol() to parse a string into an integer and to output any appropriate error messages for failed conversions. How did I do? How could/should it be done? Code works as intended if I did not miss anything.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <limits.h>

#define MAXLENGTH 1024

void retrieveStringInput(char *str, size_t buffersize)
{
    for (;;) {
        if (fgets(str, buffersize, stdin) != NULL) {
            str[strcspn(str, "\n")] = '\0';
            return;
        }
        printf("No input detected please try again...\n");
    }
}


int stringToInt(char *str)
{
    char *end = NULL;
    long number = strtol(str, &end, 10);
    if (!*end && end != str) {
        if (number <= INT_MIN || number >= INT_MAX) {
            printf("ERROR INPUT IS OUT OF RANGE FOR INTEGER VALUE, %d < number < %d\n", INT_MIN, INT_MAX);
            return 0;
        }
        return (int)number;
    }
    printf("STRING FAIILED TO CONVERT...\n");
    return 0;
}

int main(void)
{
    char number[MAXLENGTH];
    int convertNumber = 0;
    retrieveStringInput(number, sizeof number);

    if (number[0] = '0') {
        printf("Your number is: %d\n", convertNumber);
    }

    if ((convertNumber = stringToInt(number)) != 0)
        printf("YOUR NUMBER IS: %d\n", convertNumber);

    printf("Press any key to continue...\n");
    getch();
    return 0;
}
\$\endgroup\$
13
\$\begingroup\$

Compiler errors

First of all, this code does not compile with the (IMO too lenient) command line of :

gcc -Wall -Werror main.c -o strtol

Instead it gives the message that @vnp showed in their answer, that you should change if (number[0] = '0') to either if ( (number[0] = '0') ) or to if (number[0] == '0').

Having said that, the command line I usually use (and the one I put in my Makefiles) is:

-Wall -Wextra -Wfloat-equal -Wundef -Werror -fverbose-asm -Wint-to-pointer-cast -Wshadow -Wpointer-arith -Wcast-align -Wstrict-prototypes -Wcast-qual -Wmissing-prototypes -Wstrict-overflow=5 -Wwrite-strings -Wconversion --pedantic-errors -std=gnu11 -ggdb

With these arguments I get the following from my IDE:

error: conversion to ‘int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Werror=conversion] at line 15 col 24

error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] at line 45 col 9

note: place parentheses around the assignment to silence this warning at line 45 col 19

note: use '==' to turn this assignment into an equality comparison at line 45 col 19        

Without fixing these, the program does not work quite correctly:

$ ./strtol
4
Your number is: 0
Press any key to continue...


$ ./strtol
888
Your number is: 0
YOUR NUMBER IS: 88
Press any key to continue...


$ ./strtol
11
Your number is: 0
YOUR NUMBER IS: 1
Press any key to continue...

Upon further inspection, the first character is ignored in the output:

$ ./strtol
623642
Your number is: 0
YOUR NUMBER IS: 23642
Press any key to continue...


$ ./strtol
 435345345
Your number is: 0
YOUR NUMBER IS: 435345345
Press any key to continue...

Upon correcting the errors mentioned by the compiler flags, however, the bugs appear to have disappeared!

$ ./strtol
4545
YOUR NUMBER IS: 4545
Press any key to continue...


$ ./strtol
 90
YOUR NUMBER IS: 90
Press any key to continue...


$ ./strtol
asdasdasdadasdasd
STRING FAIILED TO CONVERT...
Press any key to continue...


$ ./strtol
9999999
YOUR NUMBER IS: 9999999
Press any key to continue...

Let this be a lesson to ye -- listen to the C compiler, for it knows many things :)

API

When I write in C, I try to make my functions applicable to a wide variety of uses because C is verbose, simplistic and lacks polymorphism.

Having said that, I'm not sure how I feel about the interface I find when I read your code. Because the point of this program is hardly an end unto itself, surely this has applications in other projects' #includes.

To this end, it would likely be best and most testable if functions didn't have rather unrelated side effects, like printing, which could be handled by the caller.

That is, however, quite subjective and your use-case may differ entirely.

Organisation / Style

I know you're using Windows because you call getch() without an #include <curses.h>, or perhaps that was a typo and you meant getchar() (from <stdio.h>).

So even though all the functions in windows.h are laughable with their longCamelCaseCOMNames, "idiomatic" C uses snake_case_ndrscrs_and_abbrs, not camelCase.

Moreover, in "idiomatic" modern C:

  • #include <stdbool.h> gives us the much cleaner while ( true ) { ...
  • a function purporting to call strtol (string to long int) should have type long, not int
  • code uses the bool or ssize_t return type, or writes error to a pointer passed as an argument, to indicate error rather than 0
    • this way, I don't have to have two if statements every time I want to use your function -- this is almost as bad as atoi.
  • always use braces around if statement bodies -- okay, maybe this is personal preference but I think it misleads me and other people reading your code.

How I would write it

I've been writing a lot of C lately, so here's how I'd reimplement your idea.

Mine writes a lot less STDOUT, but you could make it do more with the information the functions give you:

#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <limits.h>

#define MAXLENGTH 1024

bool    readln (char** buf, const size_t len);
char* conv_str (const char* const str, long* out);
void str_chomp (char* const str);

// read a line of STDIN
bool readln (char** buf, const size_t len) {

  if ( fgets(*buf, len > INT_MAX ? INT_MAX : (int) len, stdin) != NULL) {
    str_chomp(*buf);
    return true;
  }
  return false;

}

// try to convert a string to a number or return the erroneous part
char* conv_str (const char* const str, long* out) {
  char* end   = NULL;
  long number = strtol(str, &end, 10);

  if ( end != str && !(*end) ) {
    *out = number;
    end = NULL;
  }

  return end;
}

// cut the last newline or don't
void str_chomp (char* const str) {

  if (str && (strchr(str, '\n') != NULL) ) {
    str[ strcspn(str, "\n") ] = 0;
  }
}

int main(void) {

  char* in_buf = malloc(sizeof (char) * MAXLENGTH);

  printf("Enter a number! ");

  while ( ! readln(&in_buf, MAXLENGTH) ) {
    printf("\nEnter another! ");
  }

  long out;
  char* err = conv_str(in_buf, &out);
  if ( err == NULL) {
    printf("Your number was: %ld\n", out);

  } else {
    fprintf(stderr, "That number '%s' was junk!\n", err);
  }

  free(in_buf);
  return EXIT_SUCCESS;
}
\$\endgroup\$
  • \$\begingroup\$ I always assumed that -Wextra would cover about everything (despite there being -Wall which doesn’t do that). The additional warning flags you specify (some of which sound pretty useful, such as -Wstrict-overflow) are not included in -Wextra? \$\endgroup\$ – Jonas Schäfer Oct 5 '16 at 7:59
  • \$\begingroup\$ @JonasWielicki In the case of -Wstrict-overflow, -Wall only enables -Wstrict-overflow=1, which is not enough for me because I make silly mistakes sometimes -- I want -Wstrict-overflow=5. I don't see many others that are redundant here but I might have missed some \$\endgroup\$ – cat Oct 5 '16 at 11:47
  • \$\begingroup\$ I think that the bit that prints "Couldn't convert this part" probably ought to go to stderr, rather than stdout. It doesn't seem to be part of the normal output of the program. (That said, it's not really a normal, pipeline-style program, so perhaps the distinction is petty). \$\endgroup\$ – Toby Speight Oct 5 '16 at 12:29
  • \$\begingroup\$ @TobySpeight You're right, I guess, and furthermore that really shouldn't go inside conv_str at all because I talked about avoiding side-effects, but I feel like requiring conv_str to take a char** error pointer is redundant with strtol in the first place. \$\endgroup\$ – cat Oct 5 '16 at 13:20
  • \$\begingroup\$ @TobySpeight I've updated my answer \$\endgroup\$ – cat Oct 5 '16 at 13:28
6
\$\begingroup\$
  • Null pointer assignment

    fgets doesn't guarantee the presence of '\n' in the buffer (it only guarantees proper string termination). If the input string was too long, (so a newline hasn't been read), strscspn would return NULL.

  • stringToInt

    0 is a valid integer. Using it to signal error conditions is wrong: the caller has no means to tell if it is a legit 0 or an error, and a printed message doesn't help.

    Testing for end != str seems superfluous. If end happens to compare equal to str, it means that the very first character in str was invalid. If you still want to tell apart the empty string, it is easier (and I say cleaner) to test str[0] before calling strtol.

  • main

    You should turn on all warnings. Any decent compiler immediately spots

       warning: suggest parentheses around assignment used as truth value [-Wparentheses]
           if (number[0] = '0') {
           ^
    
\$\endgroup\$
  • \$\begingroup\$ "If the input string was too long ... strscspn would return NULL" Hmm OP is using strcspn(), not strscspn() what ever that is. If that is just a typo on your part, note that str[strcspn(str, "\n")] = '\0'; works great is '\n' does not exist. It does not return NULL in that case. It returns type size_t. \$\endgroup\$ – chux - Reinstate Monica Oct 6 '16 at 4:31
  • \$\begingroup\$ OP's use of if (!*end && end != str) { is idiomatic in C. "If end happens to compare equal to str, it means that the very first character in str was invalid. " is one of the reasons they are equal. "-+1" " (spaces) abc" are examples that the frist character was OK, but no conversion occurred. \$\endgroup\$ – chux - Reinstate Monica Oct 6 '16 at 4:35
1
\$\begingroup\$

OP's range checks are wrong.

// problems
long number = strtol(str, &end, 10);
if (!*end && end != str) {
    if (number <= INT_MIN || number >= INT_MAX) {
        printf("ERROR INPUT IS OUT OF RANGE ....

If testing int range ... (note < vs. <=, > vs. >=)

errno = 0;
long number = strtol(str, &end, 10);
if (!*end && end != str) {
    if (errno == ERANGE || number < INT_MIN || number > INT_MAX) {
        printf("ERROR INPUT IS OUT OF RANGE ....

Example strtoi()

\$\endgroup\$
  • \$\begingroup\$ See, I only notice these sorts of errors when it changes behaviour or causes warnings(errors) -- in the former case, I hopefully have unit tests, and in the latter case, I realise something grave has happened if what I thought was an innocent comparison warns me. :D \$\endgroup\$ – cat Oct 6 '16 at 11:39

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.