6
\$\begingroup\$

Background

Just got started with C and now am testing a few pages and simple problems to get the syntax right.

On Programiz they provide a few very easy problems with solutions.

This program checks if user input is a Vowel or a Consonant or none of those. I solve slightly different, but would be glad to see criticism to details I may not even be aware of.

Environment

  • For editing the code, I use Vim without any pluggins.
  • For testing code I am using Google's picoc which is a runtime much like NodeJS or Python's where -in interactive mode- you type C code and it is evaluated right away, so you don't need compilation.

Code

The code is commented to reflect what I understand of each step. Double starred comments will produce documentation. Then I mostly use /* syntax */.

/* importing modules */
#include <stdio.h>/** standard C library for input output.*/
#include <stdbool.h>/** bool data type. Not sure if this is normally used.*/
#include <ctype.h>/** character types, isalpha() function.*/

/** main takes no input, sends no output.*/
void main(void){

  /* set up */
  bool isVowel; char letter; char vowels[] = "aeiouAEIOU";

  /* take user input */
  printf("Insert a character: ");
  scanf("%c", &letter);//Store input at the address of letter.

  /* first check this is an alphabetic character */
  if(!isalpha(letter)) {printf("Not an alphabetic character\n"); }

  int nOfVowels = sizeof(vowels)/sizeof(vowels[0]);

  for(int a=0; a<nOfVowels; a++){

    if(vowels[a]==letter){ isVowel=true; }

  }

  printf("%s\n", isVowel == false ? "Consonant": "Vowel");

}
\$\endgroup\$

3 Answers 3

8
\$\begingroup\$

The minimum set of flags you should use with gcc/clang is -Wall -Wextra -pedantic and fix all warnings:

$ gcc -Wall -Wextra -pedantic main.c
main.c:6:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
    6 | void main(void){ // main takes no input, sends no output.
      |      ^~~~

You said

/** importing modules */

In C world we call this header files, not modules like in other languages such as Python.

You check if input is an alphabetic character

if(!isalpha(letter)) {printf("Not an alphabetic character\n"); }

but you carry on checking if it's a consonant or vowel which doesn't make sense:

$ ./a.out
Insert a character:    m
Not an alphabetic character
Consonant

I think you should exit with error here:

#include <stdlib.h>
(...)
  if(!isalpha(letter))
    {
      printf("Not an alphabetic character\n");
      return EXIT_FAILURE;
    }

You can check exit status in your shell:

$ ./a.out
Insert a character:      m
Not an alphabetic character
$ echo $?
1

Formatting is a matter of taste but I'd prefer variable definitions like this

bool isVowel; char letter; char vowels[] = "aeiouAEIOU";

to be divided into several lines:

bool isVowel;
char letter;
char vowels[] = "aeiouAEIOU";

You can leave for(int a=0; a<nOfVowels; a++) loop right after finding the first character that is a vowel:

  for(int a=0; a<nOfVowels; a++)
    {
      if(vowels[a] == letter)
        {
          isVowel = true;
          break;
        }
    }

Not much of a difference performance-wise when there are only 10 elements in vowels array but you could see bigger gains if larger array was used.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much. What would you define EXIT_FAILURE to? Also if you could explain the first line that'd be great. \$\endgroup\$
    – Minsky
    Feb 9, 2022 at 10:23
  • 4
    \$\begingroup\$ You don't have to define it on your own, it's defined in platform-specific manner in stdlib.h. All gcc options are described in man gcc \$\endgroup\$ Feb 9, 2022 at 10:29
5
\$\begingroup\$

The only portable return type for main() is int - although this one function is "magic" in that it doesn't need a return statement. Unlike other functions, we're allowed to just run off the end and the compiler will infer a success status for the program.

We should always check input operations, as even reading a single character may fail (e.g. if we run ./a.out </dev/null, reading will return the EOF value). In this case, we need scanf() to report exactly 1 successful conversion to be sure that letter is assigned a value:

printf("Insert a character: ");
fflush(stdout);

if (scanf("%c", &letter) != 1) {
    fputs("Input error\n", stderr);
    return EXIT_FAILURE;      /* defined in <stdlib.h> */
}

There's a classic gotcha with the use of isalpha(). It requires an unsigned character value, but char can be a signed type (e.g. in ISO-8859.1, accented letters can have negative values). The fix for this is quite simple in this program: just declare letter as unsigned char instead, and everything else can remain unchanged.

When we search vowels, we can simplify a bit. We know that sizeof vowels[0] must be 1 because it's a char, and that's the unit of sizeof. Also, we can stop searching (using break) as soon as we find a match:

for (unsigned a = 0;  a < sizeof vowels;  ++a) {
    if (vowels[a] == letter) {
        isVowel = true;
        break;
    }
}

I'd go a bit further, and make this into a function we can call from main():

static bool isVowel(unsigned char c)
{
    static const char vowels[] = "aeiouAEIOU";
    for (unsigned a = 0;  a < sizeof vowels;  ++a) {
        if (c == vowels[a]) {
            return true;
        }
    }
    return false;
}

This function can be further simplified by using the library function strchr() (in <string.h>) which returns a non-null (true) pointer if the character is found and a null (false) one if not:

static bool is_vowel(int c)
{
    static const char vowels[] = "aeiouAEIOU";
    return strchr(vowels, c);
}

When we use the result, note that isVowel == false is the same as !isVowel - or even better, we can swap the order of the conditional to use the positive sense:

isVowel ? "Vowel" : "Consonant"

Modified code

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

static bool is_vowel(int c)
{
    static const char vowels[] = "aeiouAEIOU";
    return strchr(vowels, c);
}

int main(void)
{
    printf("Insert a character: ");
    fflush(stdout);

    unsigned char letter;
    if (scanf("%c", &letter) != 1) {
        fputs("Input error\n", stderr);
        return EXIT_FAILURE;
    }

    if (isalpha(letter)) {
        const bool is_v = is_vowel(letter);
        printf("%d, %s\n",  is_v,  is_v ? "Vowel" : "Consonant");
    } else {
        printf("Not an alphabetic character\n");
    }
}

A final note on internationalisation. Although this is a simple beginner project, it can help demonstrate the problems we have when code interacts with the Real World. For example, in the word "hymn", y acts as a vowel, but the program doesn't understand this. It also won't recognise vowels such as à or ŵ. When writing real programs, it's important to get clear definitions as part of your requirements.

Don't let that complication put you off learning more C, though!

\$\endgroup\$
5
  • \$\begingroup\$ In static const char vowels[], what benefit does static provide? \$\endgroup\$
    – Harith
    May 27, 2023 at 14:55
  • 1
    \$\begingroup\$ It saves on stack and on array initialisation (in a future version, where we call the function more than once) if the compiler isn't smart enough to treat it the same as const char *const vowels = "aeiouAEIOU"; \$\endgroup\$ May 27, 2023 at 15:39
  • 1
    \$\begingroup\$ Bug: bool is_vowel('\0') returns true. strchr(vowels, c) iterates up to 11 times. for (unsigned a = 0; a < sizeof vowels; ++a) { better as for (unsigned a = 0; vowels[a]; ++a) {. Note: sizeof vowels is 11. \$\endgroup\$ May 30, 2023 at 1:27
  • 1
    \$\begingroup\$ With unsigned char letter;, consider bool is_vowel(unsigned c) for maximal portability. \$\endgroup\$ May 30, 2023 at 1:32
  • 1
    \$\begingroup\$ I'll add the usual constraint that anything else is non-portable, @chux. \$\endgroup\$ Jun 6, 2023 at 7:07
2
\$\begingroup\$

Bug: Code considers '\0' is a vowel

char vowels[] = "aeiouAEIOU"; is an array of size 11, not 10. for(int a=0; a<nOfVowels; a++){ iterates up to 11 times.

With scanf("%c", &letter);, it is possible, although tricky, to enter the null character for letter.

Suggest instead:

// for(int a=0; a<nOfVowels; a++){
for(int a=0; vowels[a]; a++){
\$\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.