CODEWITHSUNDEEP
cstring
user2785556
user2785556

Reputation: 11

What is wrong with my code that removes vowels from strings?

I'm new to programming and I've decided to do some simple programming exercises. The challenge is to remove all vowels from a user inputted string. I've already wrote the code and I don't know why it isn't working. 

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

bool isVowel(char ch)
{
    char charToBeTested = tolower(ch);
    if(charToBeTested == 'a' || 'e' || 'i' || 'o' || 'u')
        return true;
    else
        return false;
}

int main()
{
    int i;
    char formattedString[80];

    printf("Enter a string: ");
    scanf("%s", &formattedString);

    for(i = 0; i < strlen(formattedString); i++)
    {
        if(isVowel(formattedString[i]) == true)
        {
            formattedString[i] = ' ';
        }
    }

    printf("%s", formattedString);
    return 0;
}

All it is supposed to do is check every character in the string and see if it's a vowel. If it's a vowel, replace the current character with a space. I will write the function to remove the spaces later.

All help is appreciated, sorry for being such a noob!

Upvotes: 1

Views: 163

Answers (5)

Jonathan Leffler
Jonathan Leffler

Reputation: 754450

You need to test each alternative separately:

 char c = tolower(ch);
 if (c == 'a' || c == 'e' || c == 'i' || c == 'o' || c == 'u')

Your original code is:

 if (charToBeTested == 'a' || 'e' || 'i' || 'o' || 'u')

The compiler evaluates that as:

 if ((charToBeTested == 'a') || true)

because 'e' is not zero and any expression which is not zero is true. If it optimizes really thoroughly, can deduce that the whole expression will always be true, regardless of the value of charToBeTested, and hence it can reduce the entire function to return true (without ever needing to call tolower(). If it was a static function, it could even eliminate the function call altogether. Whether any compiler would actually be that aggressive is open to debate.

Upvotes: 1

Russ Clarke
Russ Clarke

Reputation: 17909

Just to add to everyones answer; as they say, you need to fix your testing criteria. Also, don't forget to check for the Uppercase version too:

if (charToBeTested == 'a' || 
    charToBeTested == 'e' || 
    charToBeTested == 'i' || 
    charToBeTested == 'o' || 
    charToBeTested == 'u' ||
    charToBeTested == 'A' || 
    charToBeTested == 'E' || 
    charToBeTested == 'I' || 
    charToBeTested == 'O' || 
    charToBeTested == 'U') {
    ...
}

Upvotes: 0

Darius Makaitis
Darius Makaitis

Reputation: 880

You'll need to rewrite your isVowel function to something like:

bool isVowel(char ch)
{
    char charToBeTested = tolower(ch);
    if(charToBeTested == 'a') {
        return true;
    } else if(charToBeTested == 'e') {
        return true;
    } else if(charToBeTested == 'i') {
        return true;
    } else if(charToBeTested == 'o') {
        return true;
    } else if(charToBeTested == 'u') {
        return true;
    } else {
        return false;
    }
}

Alternatively, you can use a switch statement to do the same thing, like this:

bool isVowel(char ch)
{
    char charToBeTested = tolower(ch);
    switch(charToBeTested) {
    case 'a':
    case 'e':
    case 'i':
    case 'o':
    case 'u':
        return true;
    default:
        return false;
    }
}

Upvotes: 1

olegarch
olegarch

Reputation: 3891

This is incorrect:

if(charToBeTested == 'a' || 'e' || 'i' || 'o' || 'u')

correct is:

if(charToBeTested == 'a' || charToBeTested == 'e' || charToBeTested == 'i' || charToBeTested == 'o' || charToBeTested == 'u')

Or, you can create static table, like:

char vowels[0x100] = {
  ['a'] = 1,
  ['e'] = 1,
  ['i'] = 1,
  ['o'] = 1,
  ['u'] = 1,
};

And test by:

if(vowels[(unsigned char)charToBeTested])

Upvotes: 1

templatetypedef
templatetypedef

Reputation: 372982

This code doesn't do what you think it does:

if (charToBeTested == 'a' || 'e' || 'i' || 'o' || 'u') {
    ...
}

C interprets this as

if ((charToBeTested == 'a') || 'e' || 'i' || 'o' || 'u') {
    ...
}

Here, the || operator is being applied directly to the character literals 'e', 'i', etc. Since C treats any nonzero value as "true," this statement always evaluates to true.

To fix this, try rewriting it like this:

if (charToBeTested == 'a' || 
    charToBeTested == 'e' || 
    charToBeTested == 'i' || 
    charToBeTested == 'o' || 
    charToBeTested == 'u') {
    ...
}

Alternatively, use a switch statement:

switch (charToBeTested) {
    case 'a': case 'e': case 'i': case 'o': case 'u':
        return true;
    default:
       return false;
}

Also, you might want to use tolower so that the testing is done case-insensitively.

Hope this helps!

Upvotes: 2

Related Questions