1

I want to delete vowels from a string and print the remaining. But my if condition doesn't behave as expected.

    #include <stdio.h>

    void filter (char *p, char *q)
    {
      while (*p != '\0')
        {
          if (*p != 'a' || *p != 'e' || *p != 'i' || *p != 'o' || *p != 'u')
        {
          *q = *p;
          q++;
        }
          p++;
        }
      *q = '\0';
    }

    int main ()
    {
      char str1[10] = "hello";
      char str2[10];
      char *p, *q;
      p = &str1[0];
      q = &str2[0];
      filter (p, q);
      printf ("%s", str2);
      return 0;
    }

I expect the output to be hll but the output was hello. I would like to know the reason for the mistake and the way to fix it.

  • 2
    I think you need `&&` instead of `||`. – Weather Vane Oct 16 '19 at 12:08
  • All the || need to be && – nicomp Oct 16 '19 at 12:08
  • 4
    Or use `!strchr("aeiou", *p)` instead of all those conditionals. – Shawn Oct 16 '19 at 12:10
  • 1
    This has the _[parts of a good question](https://stackoverflow.com/help/on-topic)_. Why the down vote I wonder? – ryyker Oct 16 '19 at 12:13
  • 2
    *Or use `!strchr("aeiou", *p)`...* That also has the advantage that the filter character(s) don't have to be hard coded. – Andrew Henle Oct 16 '19 at 12:15
  • @AndrewHenle you could hammer, I spent my cv already ^ – Antti Haapala -- Слава Україні Oct 16 '19 at 12:17
  • @ryyker not every umpteenth duplicate of the same question is worth keeping. Most notably the question listing says: *"Questions about a problem that can no longer be reproduced or that was caused by a simple typographical error. **This can often be avoided by identifying and closely inspecting the shortest program necessary to reproduce the problem before posting.**"* If the `if` condition does not behave as expected there would be substantially shorter [mre] for the question. Note that the problem is exactly addressed in the *other* duplicate question which *too*, alike, "uses a pointer". – Antti Haapala -- Слава Україні Oct 16 '19 at 12:23
  • 1
    @AnttiHaapala - you linked aclosed question, which in turn links a closed question. If you follow the _has an answer here_ links, the first _[non-closed question](https://stackoverflow.com/questions/6115801/how-do-i-test-if-a-variable-does-not-equal-either-of-two-values)_ is so far removed from this one as not to be recognized as the same topic, or even same language. – ryyker Oct 16 '19 at 12:25
  • @ryyker but it **is** a duplicate. That's the question you should reopen, not this. It had better answers too. – Antti Haapala -- Слава Україні Oct 16 '19 at 12:25
  • 2
    Still, it's not very welcoming to shower a new user with downvotes. The question is well presented even if duplicates were not found. – Weather Vane Oct 16 '19 at 12:44

4 Answers4

6

This here:

if (*p != 'a' || *p != 'e' || *p != 'i' || *p != 'o' || *p != 'u')

Should be:

if (*p != 'a' && *p != 'e' && *p != 'i' && *p != 'o' && *p != 'u')

On a side note, you can leave out the char *p, *q; part. Just do it like this:

int main()
{
    char str1[10] = "hello";
    char str2[10];
    filter(str1, str2);
    printf("%s", str2);
    return 0;
}
Blaze
  • 16,736
  • 2
  • 25
  • 44
5

Check your if condition. It is always true. Use && instead of ||. In your example an 'e' does not equal an 'a' and so the condition becomes true and the char is added to the resulting string. Try this:

if (*p != 'a' && *p != 'e' &&*p != 'i' && *p != 'o' && *p != 'u')

TobiSH
  • 2,833
  • 3
  • 23
  • 33
2

The condition

if (*p != 'a' || *p != 'e' || *p != 'i' || *p != 'o' || *p != 'u')

is wrong. For example when *p is equal to 'e' that is when *p is a vowel the expression *p != 'a' yields true and this is the result of the full condition.

You have to write the if statement like

if (*p != 'a' && *p != 'e' && *p != 'i' && *p != 'o' && *p != 'u')

or like

if ( ! ( *p == 'a' || *p == 'e' || *p = 'i' || *p != 'o' || *p != 'u') )

Also following the convention for the standard C string functions the function should return pointer to the result string. And the first parameter shall have the qualifier const because the pointed string is not changed in the function.

Instead of using the long expression in the if condition you could use the standard C function strchr to check whether the pointed character is a vowel or not.

Here is a demonstrative program that shows how the function can be defined.

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

char * filter( const char *p, char *q )
{
    const char *vowels = "aeiou";
    char *result = q;

    do
    {
        if ( *p == '\0' || strchr( vowels, *p ) == NULL ) *q++ = *p;
    } while ( *p++ );

    return result;
}

int main(void) 
{
    char *str1 = "hello";
    char str2[10];

    puts( filter( str1, str2 ) );

    return 0;
}

Its output is

hll

And you could modify the function in the way that it would skip an upper case vowels.

For example

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

char * filter( const char *p, char *q )
{
    const char *vowels = "aeiou";
    char *result = q;

    do
    {
        if ( *p == '\0' || strchr( vowels, tolower( ( unsigned char )*p ) ) == NULL ) 
        {
            *q++ = *p;
        }           
    } while ( *p++ );

    return result;
}

int main(void) 
{
    char *str1 = "HELLO";
    char str2[10];

    puts( filter( str1, str2 ) );

    return 0;
}

The program output is

HLL
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
0

The other answers all address the logic error clearly.
This one simply offers a thought on readability or style. (and a small logic improvement.)

Constructs such as the one used in your code to produce a result can become large and distracting, making the flow of your code less readable. Once the logic, and accuracy of a construct has been achieved, it is sometimes possible to move that code out of the main body, and replace it with something more readable that accomplishes the same thing. for example, your code could be changed to:

    void filter (char *p, char *q)
    {
      while (*p != '\0')
        {
          if (NOT_VOWEL(*p))
        {
          *q = *p;
          q++;
        }
          p++;
        }
      *q = '\0';
    }

By using a C Macro:

Where the macro is defined as:

//either in a header file, or somewhere at 
//the top of of the .c file where it is used.
#define NOT_VOWEL(x) (x != 'a' && x != 'e' && x != 'i' && x != 'o' && tolower(x) != 'u')  

Or, with a small improvement:

#define NOT_VOWEL(x) (tolower(x) != 'a' && tolower(x) != 'e' && tolower(x) != 'i' && tolower(x) != 'o' && tolower(x) != 'u')  

(Note, this variation of the macro includes the tolower() function, to exclude uppercase vowels as well.)

ryyker
  • 22,849
  • 3
  • 43
  • 87