1
\$\begingroup\$

New to the C language. Which design mistakes have I made in my first C program? Or what can I improve? Any little detail is appreciated.

Note: I have experience in multiple coding languages. (this is the beginning of my C journey)

Description of the program: checks if a string/array of characters is a bit string. Example: "010100" -> valid.

#include <stdio.h>
#include <string.h>

int main() {
    char str[100];
    fgets(str, 100, stdin);

    int len = strlen(str);
    int len2 = len - 1;
    int count = 0;

    //loop through string
    for(int i = 0; i < len2; i++) {
        switch (str[i]) {
            case '0':
            case '1':
                count++;
                break;
        }
    }

    //is string a bit string
    if (count == len2) 
        printf("Valid!\n");
    else
        printf("Not Valid...\n");

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ Use len i.o. len2, as str[len] == '\0', the function strlen giving size - 1. \$\endgroup\$
    – Joop Eggen
    Jan 21, 2019 at 10:46

3 Answers 3

4
\$\begingroup\$

Ultimately, you are looking to see if any character is not either '0' or '1'. The moment you find such a character, you can break the loop immediately:

int isBitString = 1;
for(int i = 0; i < len2; i++) {
    if (str[i] != '0' && str[i] != '1') {
        isBitString = 0;
        break;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ That is correct Abigail. And Joe C; your break loop is much more neater than my variant. Appreciate the input. \$\endgroup\$ Jan 20, 2019 at 17:47
  • \$\begingroup\$ Fair enough, I've edited my answer to account for that. \$\endgroup\$
    – Joe C
    Jan 20, 2019 at 17:49
0
\$\begingroup\$

Take care with <stdio.h>

Output and (especially) input can be quite error-prone activities. Users and systems have an annoying habit of interacting with your program in unexpected ways. So we definitely need to check the return value of fgets(); if it's a null pointer, then we didn't read any input, and the contents of str are still uninitialised (so we should print an error message to stderr and return a non-zero status instead of continuing).

Another possibility is that the user enters more than 98 digits, in which case we ignore the 99th and subsequent ones. One approach to this is to check whether the last character of the string is a newline; if it's not, then we'll need to read another block of sizeof str characters and continue. A simpler alternative is to read a character at a time rather than trying to read whole lines; although you might think that's less efficient, the stdio library buffers input so that it's not noticeably slower.

Early return

We can return before reading to end of string if we find an invalid character. Then we know that if we reach the final newline, then all of the input was valid.

A simplification

To print a fixed string ending with newline, we can use puts() instead of printf().


Taking the above suggestions into account, I end up quite a different program:

#include <stdio.h>

int main(void)
{
    while (1) {
        switch (getc(stdin)) {
        case EOF:
            fputs("Read error!\n", stderr);
            return 1;

        case '0':
        case '1':
            /* valid character */
            continue;

        case '\n':
            /* end of a valid input line */
            puts("Valid!");
            return 0;

        default:
            /* invalid character */
            puts("Not Valid...");
            return 0;
        }
    }
}

What we now have is an extremely simple state machine. The starting state is "valid so far", and the end states are "all valid" and "error". The transitions are all in the switch statement.

Note that like the original code in the question, this program considers empty input to be a valid binary string. That may or may not be what's wanted.

\$\endgroup\$
0
\$\begingroup\$

Avoid assumptions

Code subtracts 1 with no explanation. A usual reason to do so here is an assumed '\n' at the end of str.

fgets(str, 100, stdin);
int len = strlen(str);
int len2 = len - 1;  // Why - 1?

Consider why this is not always correct.

1) User input was long like "000...(98 total zero characters)...000x\n". fgets() would read the first 99 characters into str and code would identify that as a good string, even though it had a x in str.

2) User input was "012" and then input was closed. fgets() would read "012" into str and code would identify that as a good string, even though it had a 2 in str.

3) Some hacker is messing with the program and inputs "0z\0\n" and code would identify that as a good string, even though input had a 'z'.

4) fgets() suffered a rare input error. The state of str in indeterminate. strlen(str) leads to UB.

To fix 1,2, 4 and partially #3

if (fgets(str, 100, stdin) == NULL) {
  printf("End of file or input error\n");
  return 0;
}
int len = strlen(str);
if (len > 0 && str[len-1] == '\n') str[--len] = '\0';
int len2 = len;

Code passes the empty line

I'd expect an input of "\n" to warrant a "Not Valid...\n"

Goal not cleanly met

Given the task of "Is a string a bit string", code should have a function like

bool is_bit_string(const char *s);

Instead OP's code answered "Is a line of input a bit string".

In C:

A string is a contiguous sequence of characters terminated by and including the first null character.

A text stream is an ordered sequence of characters composed into lines, each line consisting of zero or more characters plus a terminating new-line character. Whether the last line requires a terminating new-line character is implementation-defined.

'a', 'b', 'c', '\0' is a string.
'a', 'b', 'c', '\n' is a line.

Separate core function

The input/output should not be part of "Is a string a bit string". Perhaps:

#include <stdbool.h>

bool is_bit_string(const char *s) {
  if (*s == '\0') {
    return false;
  }
  while (*s >= '0' && *s <= '1') {
    s++;
  }
  return *s == '\0';
}

Test code

Move testing of "Is a string a bit string" away from is_bit_string() definition. perhaps even in separate .c files.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#define TEST_SZ 256

bool is_bit_string(const char *s);

int main(void) {
  char str[TEST_SZ];
  if (fgets(str, sizeof str, stdin) == NULL) {
    fprintf(stderr, "End of file or input error\n");
    return EXIT_FAILURE;
  } 

  // Lop off potential \n
  str[strcspn(str, "\n")] = '\0';

  if (is_bit_string(str)) {
    printf("Valid!\n");
  } else {
    printf("Not Valid...\n");
  }

  return EXIT_SUCESS;
}
\$\endgroup\$

Your Answer

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

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