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.
-
3\$\begingroup\$ Is it intentional that "123-456" passes the test as an integer? \$\endgroup\$Martin R– Martin R2019年01月12日 18:43:40 +00:00Commented Jan 12, 2019 at 18:43
-
\$\begingroup\$ Some test cases to consider: "0", "0-0", "-0", "+9", "--9" \$\endgroup\$Edward– Edward2019年01月12日 21:43:11 +00:00Commented Jan 12, 2019 at 21:43
2 Answers 2
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円';
}
-
\$\begingroup\$ @Vlastimil I did not forget.
main()
is special. The return is optional. Is It Necessary to Return a Value in Main()? IMO, it is better to include. Follow your group's style guide. \$\endgroup\$chux– chux2019年01月13日 05:42:51 +00:00Commented Jan 13, 2019 at 5:42
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)
-
-
\$\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– Toby Speight2019年01月14日 15:34:17 +00:00Commented Jan 14, 2019 at 15:34