4
\$\begingroup\$

I have started to learn C. At the time of writing I don't know anything about the language. The following I wrote today in a few hours. Though I do have a lot of programming experience, I find C hard to learn. That may be only a temporary impression, however.


The purpose of the following function is to determine if the string, whatever its length is, contains an integer number. There might be many ways, and/or I might have done some rookie mistakes.


#include <stdbool.h> // bool type, true / false values, ...
#include <stdio.h> // printf()
#include <ctype.h> // isdigit()
#include <string.h> // strlen(), strncmp(), ...

bool string_contains_integer(const char *str)
/*
    This function iterates through an array of chars,
    and checks each character to be a digit.

    Returns true if the string contains a number (string of digits);
    Returns false if the string contains another character(s).

    Starting "-" gets ignored, as we accept negative numbers too.
*/
{
    // I'd like to avoid repeated strlen() calls
    int string_length = strlen(str);

    // if the string is empty, return right away
    if (string_length == 0)
        return false;

    bool negative_sign = false;

    // iterate through all characters in the string
    for (int i = 0; i < string_length; i++)
    {
        // extract character on index
        char chr = str[i];

        // repeated presence of negative sign is handled here
        if (strncmp("-", &chr, 1) == 0)
        {
            // if the negative sign gets detected
            // for the second time, return right away
            if (negative_sign)
                return false;

            // once a negative sign is detected, we make note of it here
            negative_sign = true;

            // now we want to skip the check for a digit
            continue;
        }

        // this is actually the core part checking on digits
        if (!isdigit(chr))
            return false;
    }

    // If we got here, then all the characters are digits,
    // possibly starting with a negative sign.
    return true;
}

int main()
{
    // the function can handle very big numbers as shown here
    if (string_contains_integer("-123456789123456789123456789123456789123456789123456789123456789"))
    {
        printf("%s\n", "PASS: Input is a number.");
    }
    else
    {
        printf("%s\n", "FAIL: Input is not a number!");
    }
}

This program I compile on Linux Mint 19.1 as follows:

gcc-8 -std=c18 -Wall -Wextra -Werror -Wpedantic -pedantic-errors -o bigInteger bigInteger.c

Would you be that kind to review the code? However, there might be alternative ways of doing the same thing, you may, of course, suggest them, but my primary goal now is to check upon my learning curve and to fix bugs of course, optimize this code, etc.

\$\endgroup\$
  • 3
    \$\begingroup\$ Is it intentional that "123-456" passes the test as an integer? \$\endgroup\$ – Martin R Jan 12 at 18:43
  • \$\begingroup\$ Some test cases to consider: "0", "0-0", "-0", "+9", "--9" \$\endgroup\$ – Edward Jan 12 at 21:43
5
\$\begingroup\$

Good observation

Many new C programmers do not realize that n = strlen(s) when called in a loop can take code from linear performance O(n) to worse O(n*n) performance when s may change or with weak compilers.

// I'd like to avoid repeated strlen() calls
int string_length = strlen(str);  // Good!!

Good use of const

Nice. string_contains_integer(const char *str)

Nicely formatted code

I hope you are using an auto formatter. Life is too short for manual formatting.


"whatever its length is"

Strings can have a length that exceeds INT_MAX. Note that strlen() returns type size_t. That is the right-sized type for array indexing and sizing. Note: size_t is some unsigned type.

// int string_length = strlen(str);
// for (int i = 0; i < string_length; i++)

size_t string_length = strlen(str);
for (size_t i = 0; i < string_length; i++)

Expensive sign compare

Rather than call strncmp(), just compare characters.

// char chr = str[i];
// if (strncmp("-", &chr, 1) == 0)
if (str[i] == '-')

OP's code is tricky here. strncmp() can handle non-strings (which is what &chr points to) as long as they do not exceed n. With n==1, code is effectively doing a char compare.

Sign at the beginning

Conversion of strings to numeric values allow a leading sign, either '-', '+' or none. This code errantly allows a '-' someplace. @Martin R.

Just start with

bool string_contains_integer(const char *str) {
  if (*str == '-' || *str == '+') {
    str++;
  }
  ....

  // Other sign code not needed

strlen() not needed.

Rather than i < string_length, just test str[i] != '\0'. This speeds up detection of non-numeric strings as the entire string does not need to be read.

is...(int ch) quirk

is...() functions are specified for int values in the unsigned char range and EOF. On common platforms where char is signed, isdigit(chr) risks undefined behavior (UB) when chr < 0. Best to cast here.

// isdigit(chr)
isdigit((unsigned char) chr)

{ block }

Of coding style, I recommend to always use {} after if,else, even if one line.

//if (string_length == 0)
//    return false;

if (string_length == 0) {
    return false;
}

Minor: Simplification

To simply prints a line, code could use puts(). But what you did is OK. Note puts() appends a '\n'.

// printf("%s\n", "PASS: Input is a number.");
puts("PASS: Input is a number.");

Alternative

This does not use array indexing. Just increment the pointer as needed.

bool string_contains_integer_alt(const char *str) {
  if (*str == '-' || *str == '+') {
    str++;
  }

  if (*str == '\0') {
    return false;
  }

  while (isdigit((unsigned char)*str)) {
    str++;
  }

  return *str == '\0';
}
\$\endgroup\$
2
\$\begingroup\$

You already have a good review by chux; I'll not repeat the observations from there. Instead, I'll highlight a couple of opportunities to advance beyond "beginner" level C.

Become more comfortable with pointers

Many C beginners seem to be intimidated by pointers. That can lead to clumsy non-idiomatic C using indexing instead. Whilst that's functionally correct, it will look unwieldy to the experienced C programmer, and will also hinder your ability to read code written in a more pointer-oriented style.

It's worth practising your pointer code until your are completely confident you can both write and understand that style.

Learn what's in the Standard Library

The C Standard Library contains many functions to make your life easier. You're using isdigit() which is great; another function that may help us is strspn(), which can replace the loop for us (but we'll have to specify the allowed digits, which loses us some of the locale-independence that isdigit() gives us):

bool string_contains_integer(const char *str)
{
    if (*str == '-') {
        ++str;
    }

    size_t digit_count = strspn(str, "0123456789");
    return digit_count > 0
        && str[digit_count] == '\0';
}

The digit_count > 0 test ensures we have at least one digit, and str[digit_count] == '\0' checks that the first non-digit is the the end of the string.

Add more tests

It's worth writing a small helper function:

static bool test_contains_integer(bool expected, const char *str)
{
    if (expected == string_contains_integer(str)) {
        return true;
    }
    fprintf(stderr, "FAIL: Expected %s but got %s for input %s\n",
            expected ? "true" : "false",
            expected ? "false" : "true",
            str);
    return false;
}

When we have a lot of tests, it helps to produce output only for the failures. We can count how many pass and how many fail:

int main(void)
{
    /* counts of failure and pass */
    int counts[2] = { 0, 0 };

    ++counts[test_contains_integer(false, "")];
    ++counts[test_contains_integer(false, "-")];
    ++counts[test_contains_integer(true, "0")];
    ++counts[test_contains_integer(true, "-0")];  /* questionable */
    ++counts[test_contains_integer(true, "-00")]; /* questionable */
    ++counts[test_contains_integer(true, "1")];
    ++counts[test_contains_integer(false, "1a")];
    ++counts[test_contains_integer(true, "-10")];

    printf("Summary: passed %d of %d tests.\n",
           counts[true], counts[false] + counts[true]);

    return counts[false] != 0;
}

Finally, a minor nit: prefer to declare a main that takes no arguments, rather than one that takes unspecified arguments:

int main(void)
\$\endgroup\$
  • \$\begingroup\$ Thank you very much. Though I already decided not to actually code in pure C, and learn C++ basics instead. \$\endgroup\$ – Vlastimil Jan 14 at 11:18
  • \$\begingroup\$ The ancient orders of Ones` Complement Preservation Society and Sign Magnitude Forever wish to lodge a protest of the besmirch-ment of "-0")]; /* questionable */. "-0" are people too. ;-) \$\endgroup\$ – chux Jan 14 at 14:50
  • \$\begingroup\$ @chux, protest noted ;-) In my defence, I'll point out that that I actually meant that we should question/confirm that the behaviour is the one that's desired; "questionable" != "wrong" when I'm reviewing. \$\endgroup\$ – Toby Speight Jan 14 at 15:34

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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