Counting vowels c++

Question

While user inputs 10 alphabets, program should tell how many of them are vowels. I have written this code:

while (count<10)
    {
        cin >> n;
        count++;
        if (n == 'A' || n == 'a' && n == 'E' || n == 'e' && n == 'I' || n == 'i' && n == 'O' || n == 'o' && n == 'U' || n == 'u')
            {
                total++;
            }
    }

cout <<  total << endl;

This generates output of 0 even if there are vowels entered by user. something wrong?

Replace all `&&` by `||`. Also what is the type of `n`? Provide a [mre]. — n314159, Nov 28 '19 at 16:42
Your code checks to see if a character is *simultaneously all five vowels*! — Adrian Mole, Nov 28 '19 at 16:45
Please, read this literally: No character can be an `A` or `a` and an `E` or `e` and ... Output of 0 is the obviously correct answer for any input. ;-) — Scheff's Cat, Nov 28 '19 at 16:45
@n314159 sorry I had this view in mind that if I put || (or) there, it will just check the first condition and will add A/a to the total without checking rest of them. — scypx, Nov 28 '19 at 16:47
Reduce your comparisons by half, by using `std::toupper` or `std::tolower`. — Thomas Matthews, Nov 28 '19 at 17:12
@Thomas Matthews If using `std::tolower` or `std::toupper` one should make the `n` an `unsigned char`. — n314159, Nov 28 '19 at 17:30
@n314159: Please document the reference where you got this idea. My understanding is that [`std::toupper`](https://en.cppreference.com/w/cpp/string/byte/toupper) takes a *signed* int as the parameter. Passing a `char` will automatically get converted. — Thomas Matthews, Nov 28 '19 at 17:34
@Thomas Matthews Sorry, I missread the reference you linked a bit. But compare this with [`std::isdigit`](https://en.cppreference.com/w/cpp/string/byte/isdigit). It has the same signature and has a note that one should cast to `unsigned char`. — n314159, Nov 28 '19 at 17:41
@n314159: Two different functions. Please quote the section of the C++ standard where the functions must have the same signature. (You may want to browse to the sections about `std::toupper` and `std::tolower`). — Thomas Matthews, Nov 28 '19 at 17:46
@Thomas Matthews Sorry, I was unclear. I don't claim anymore that `std::tolower` needs an `unsigned char`. I am just confused why `std::isdigit` needs that and `std::tolower` does not, when both are functions taking an `int` that has to be representable by `unsigned char` (this precondition is stated on cppreference). — n314159, Nov 28 '19 at 17:52
@n314159: You should ask this as a separate question. Please also see the function versions declared in ``, [`std::tolower`](https://en.cppreference.com/w/cpp/locale/tolower), [`std::isdigit`](https://en.cppreference.com/w/cpp/locale/isdigit) — Thomas Matthews, Nov 28 '19 at 18:01

Ryan Haining · Accepted Answer · 2019-11-28T17:09:10.763

7

Let's start by cutting down the condition down a bit, to only look at a and e

if (n == 'A' || n == 'a' && n == 'E' || n == 'e')

and then only considering the lowercase letters for simplicity (but retains the problem)

if (n == 'a' && n == 'e')

if you read this out loud it says "if n is 'a' AND n is 'e'". The "AND" (from the && operator) means that both conditions must be true. You've created an impossible condition here, if n is 'a', then it is not 'e', so you get if (true && false) - which is false. If n is 'e', then it is not 'a', so you get if (false && true).

Simply replace all of your && (and) operators with || (or) operators to have the condition be true if at least one of your equality comparisons is true.

if (n == 'A' || n == 'a' || n == 'E' || n == 'e' 
    || n == 'I' || n == 'i' || n == 'O' || n == 'o' 
    || n == 'U' || n == 'u')

There are some ways to simplify the condition.

One is to add a #include <cctype> and use std::tolower to convert n to lowercase, then you only need to compare against lowercase characters.

n = std::tolower(n);
if (n == 'a' || n == 'e' || n == 'i' || n == 'o' || n == 'u')

Another, less repetitive approach is to create a std::string with all the vowels in it, and then see if it contains n, #include <string> up top.

std::string vowels = "aeiouAEIOU";
while (/*...*/) {
   // ...
   if (vowels.find(n) != std::string::npos) {
     ++total;
   }
}

As n314159 pointed out, if you are using C++17 or later, you can use a std::string_view instead, which is cheaper. #include <string_view>

static constexpr std::string_view vowels = "aeiouAEIOU";   
while (/*...*/) {
   // ...
   if (vowels.find(n) != std::string_view::npos) {
     ++total;
   }
}

edited Nov 28 '19 at 17:09

answered Nov 28 '19 at 16:46

Ryan Haining

35,360
15
114
174

I overlooked that `&&` has higher precedence than `||`. However, even considering this, it doesn't become better... ;-) – Scheff's Cat Nov 28 '19 at 16:47
On C++17 I would recommend substituting the `std::string` by an `constexpr std::string_view` since it may be cheaper and the compiler should be able to optimize it more. – n314159 Nov 28 '19 at 17:01
@scypx Please, don't forget to mark an [answer](https://stackoverflow.com/help/someone-answers). – Scheff's Cat Nov 28 '19 at 17:03
Hm, it would be interesting if `std::tolower` and then `find` on a smaller string is faster than `find` the long string. I mean, `std::lower` (if it is unsafe) should implementable by one subtraction. Not that it is in any way relevant^^ – n314159 Nov 28 '19 at 17:45
@scypx A solution without using `std::tolower` but bit operations: `if ((n|=0x20) && (!(n^'a') || !(n^'e') || !(n^'i') || !(n^'o') || !(n^'u')))` – urbanSoft Nov 28 '19 at 22:09
@urbanSoft doesn't seem any better than `==` comparison, what does this handle that I'm missing – Ryan Haining Nov 29 '19 at 17:24
@RyanHaining It's not about beeing better rather than diffrent. Key part is `(n|=0x20)` which always sets bit no. 6 in `n`. When dealing with ANSI letters `n` will become (or stay) lowercase so there's no need to use `std::tolower`. To my mind it's an awesome feature of th ANSI table ([additional reading](https://catonmat.net/ascii-case-conversion-trick)). – urbanSoft Dec 02 '19 at 09:02
@urbanSoft I get that, I'm not following why you are doing `!(n^'a')` instead of `n == 'a'` – Ryan Haining Dec 02 '19 at 22:21
@RyanHaining For no reason other than personal favour. Since I started with a bit operation i simply stuck with them. – urbanSoft Dec 04 '19 at 08:19
1

@urbanSoft I would encourage you to use `==` when you mean "equals" if others will be reading your code. You will make a lot of enemies using unnecessary bit operations like this. Obviously nothing really matters if all you do is solo projects. – Ryan Haining Dec 04 '19 at 19:11

Kostas · Answer 2 · 2019-11-28T17:12:48.693

1

I recommend using a switch statement with fall-through, which is more readable but also potentially more efficient (may be implemented with jump table).

int count = 10;
while (count--) {
    switch(std::tolower(n)) {
        case 'a': case 'e': 
        case 'i': case 'o': 
        case 'u': total ++; break;
        default:;
    }
}

edited Nov 28 '19 at 17:12

answered Nov 28 '19 at 16:52

Kostas

4,061
1
14
32

1

You should add `[[ fallthrough ]]` or get annoyed by compiler warnings. – n314159 Nov 28 '19 at 16:59
@n314159 This doesn't give warning with either g++ or clang++ (even with -Weverything flag). I'm guessing they see the empty `case` body and they assume it's explicit enough. – Kostas Nov 28 '19 at 17:04
@n314159 Hmm, you're actually right it sometimes gives a warning. Only for the non-empty `'u'` case though, so fixed it : ) – Kostas Nov 28 '19 at 17:13

Counting vowels c++

2 Answers2