1
\$\begingroup\$

Link to original question: Successful use of strtol() in C

After getting excellent feedback from the code review community I attempted to redo my string to long conversion program! I believe I enhanced it significantly(Most ideas from answers on my last post) but I did add my own 'style' to it and am looking to see if I missed anything or need to change the way I implemented the solution.

#include "stdafx.h"
#include <stdio.h>
#include <stdbool.h>
#include <limits.h>
#include <stdlib.h>

#define MAXLENGTH 1024

bool readline(char** buf, const size_t buf_len);
char* str_to_long(const char* const buf, long* out);
void str_cmp(char* str);

int main() {
    char* str = malloc(sizeof(char) * MAXLENGTH);
    long cnvt_numb = 0;
    printf("Enter a number: ");
    while (!readline(&str, MAXLENGTH)) {
        printf("Try again...\n");
    }
    char* err = str_to_long(str, &cnvt_numb);
    if (err == NULL) {
        if (cnvt_numb == LONG_MAX) {
            fprintf(stderr, "INPUT OUT OF RANGE...\n");
        }
        else {
            printf("Your number was: %ld\n", cnvt_numb);
        }
    }
    else {
        fprintf(stderr, "That number '%s' was not convertable...\n", err);
    }

    free(str);

    printf("Press any key to continue...\n");
    getch();
    return EXIT_SUCCESS;
}

bool readline(char** buf, const size_t buf_len) {
    if ((fgets(*buf, buf_len, stdin)) != NULL) {
        str_cmp(*buf);
        return true;
    }
    return false;
}

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

//if returns anything other than NULL, conversion failed
char* str_to_long(const char* const str, long* out) {
    char* end = NULL;
    long number = strtol(str, &end, 10);
    if ((!*end) && (end != str)) {
        *out = number;
        end = NULL;
    }
    return end;
}
\$\endgroup\$
4
\$\begingroup\$

#include

You didn't supply the contents of "stdafx.h", but I was able to compile simply by removing that include, so you probably don't need it.

You do seem to be missing a necessary include of <string.h>.

malloc()

In main() you allocate a fixed length buffer on the heap. There's no advantage over stack allocation (unless you're compiling for a system with a tiny stack), and no need to multiply by 1: sizeof (char) is necessarily 1, by definition:

char str[MAXLENGTH];

Also, I note that there aren't many platforms that support 3400-bit longs, so you could probably make the input buffer a bit smaller.

Having done this, your call to readline() can be

readline(&str, sizeof str)

readline()

You take a pointer to pointer, but don't ever change the pointed-to value, so you could introduce a const between the stars, or (better) remove a level of indirection:

bool readline(char *buf, const size_t buf_len) {
    if (fgets(buf, buf_len, stdin)) {
        str_cmp(buf);
        return true;
    }
    return false;
}

Also, it's not clear whether you want to return true for a partial-line read; it's probably worth a comment to clarify this.

str_cmp()

You should remember the value returned by strchr, rather than recomputing it:

void str_cmp(char* str) {
    char *nl = strchr(str, '\n');
    if (nl)
        *nl = '\0';
}

I also prefer to write '\0' in preference to 0 to show we're dealing with string characters here.

Since it's only used from one place, we can inline this function. It's a judgement call, but I don't see enough benefit from having this separate.

back to main():

This code doesn't do what it claims:

char* err = str_to_long(str, &cnvt_numb);
/* ... */
fprintf(stderr, "That number '%s' was not convertible...\n", err);

The value you get back as err isn't the string - it's only the portion after the parseable digits. So for input such as 10x, you'll just print x; it's not obvious that's what you want. I'd show the whole string to the user:

fprintf(stderr, "That number '%s' was not convertible...\n", str);

You probably don't want to return EXIT_SUCCESS in that case, either.

You test for output of LONG_MAX, but not LONG_MIN. Actually, both of those are legitimate outputs, so by not testing errno, you are unnecessarily reducing the range you accept.

There's a call to getch() but that's not declared in any of the headers. Perhaps you meant getchar()? Either way, asking for extra input just to exit the program is user-hostile, and I'd just excise that part.


Putting it all together

Here's what I ended up with:

#include <errno.h>
#include <stdio.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

#define MAXLENGTH 40            /* supports 128-bit long */


bool readline(char* buf, const size_t buf_len) {
    if (!fgets(buf, buf_len, stdin))
        return false;
    char *nl = strchr(buf, '\n');
    if (nl)
        *nl = '\0';
    return true;
}

// returns NULL on success, or the unparseable portion of input on failure
char* str_to_long(const char* const str, long* out) {
    char* end;
    errno = 0;
    *out = strtol(str, &end, 10);
    if (!*end && end != str)
        end = NULL;
    return end;
}


int main() {
    char str[MAXLENGTH];
    long cnvt_numb = 0;

    printf("Enter a number: ");
    while (!readline(str, sizeof str)) {
        printf("Try again...\n");
    }

    char* err = str_to_long(str, &cnvt_numb);
    if (err) {
        fprintf(stderr, "That number '%s' was not convertible...\n", str);
        return EXIT_FAILURE;
    } else if (errno == ERANGE) {
        fprintf(stderr, "INPUT OUT OF RANGE...\n");
        return EXIT_FAILURE;
    } else {
        printf("Your number was: %ld\n", cnvt_numb);
        return EXIT_SUCCESS;
    }
}

It accepts input from -9223372036854775808 to 9223372036854775807 (on this system, where long is 64 bits), and correctly reports a range error for -9223372036854775809 and for 9223372036854775808.

12345aoeu is rejected with

That number '12345aoeu' was not convertible...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 3400 is a magic number in the review. Better to show the basic relation ship to OP's MAXLENGTH. (e.g. "... there aren't many platforms that need a 1024 char buffer (about 3400 bit long ). My recommended buffer size is twice the number of characters needed to encode any long. #define INTEGER_STR_SIZE(t) (CHAR_BIT*sizeof(t)/3 + 3) #define MAXLENGTH (INTEGER_STR_SIZE(long)*2) \$\endgroup\$ – chux - Reinstate Monica Oct 11 '16 at 12:34
  • 1
    \$\begingroup\$ Detail: 40 is certainly ample for 64-bit long. Yet #define MAXLENGTH 40 /* supports 128-bit long */ --> 40 is insufficient for reading strings representing all 128 bit long. Need at least ceil(127_value_bits*ln2 + 1 (sign) + 1 (\n) +1 \0) --> 42. Recommend formula rather than fixed computed values. \$\endgroup\$ – chux - Reinstate Monica Oct 11 '16 at 12:54
  • \$\begingroup\$ @chux, yes, I failed to allow space for the newline, and for a possible leading - . And our error-handling isn't great when the user enters an overlength value (really we should readline() until we reach \n, and discard that before re-asking). \$\endgroup\$ – Toby Speight Oct 11 '16 at 13:14
  • \$\begingroup\$ This is kinda a review of my review so thank you! \$\endgroup\$ – cat Nov 2 '16 at 23:03

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.