C# Count Vowels

Question

I am learning to program C# and I am trying to count the vowels. I am getting the program to loop through the sentence, but instead of returning vowel count, it is just returning the length of the string. Any help would be greatly appreciated.

    static void Main()
    {
        int total = 0;

        Console.WriteLine("Enter a Sentence");
        string sentence = Console.ReadLine().ToLower();

        for (int i = 0; i < sentence.Length; i++)
        {
            if (sentence.Contains("a") || sentence.Contains("e") || sentence.Contains("i") || sentence.Contains("o") || sentence.Contains("u"))
            {
                total++;
            }
        }
        Console.WriteLine("Your total number of vowels is: {0}", total);

        Console.ReadLine();
    }

Solution 1

Right now, you're checking whether the sentence as a whole contains any vowels, once for each character. You need to instead check the individual characters.

   for (int i = 0; i < sentence.Length; i++)
    {
        if (sentence[i]  == 'a' || sentence[i] == 'e' || sentence[i] == 'i' || sentence[i] == 'o' || sentence[i] == 'u')
        {
            total++;
        }
    }

That being said, you can simplify this quite a bit:

static void Main()
{
    int total = 0;
    // Build a list of vowels up front:
    var vowels = new HashSet<char> { 'a', 'e', 'i', 'o', 'u' };

    Console.WriteLine("Enter a Sentence");
    string sentence = Console.ReadLine().ToLower();

    for (int i = 0; i < sentence.Length; i++)
    {
        if (vowels.Contains(sentence[i]))
        {
            total++;
        }
    }
    Console.WriteLine("Your total number of vowels is: {0}", total);

    Console.ReadLine();
}

You can simplify it further if you want to use LINQ:

static void Main()
{
    // Build a list of vowels up front:
    var vowels = new HashSet<char> { 'a', 'e', 'i', 'o', 'u' };

    Console.WriteLine("Enter a Sentence");
    string sentence = Console.ReadLine().ToLower();

    int total = sentence.Count(c => vowels.Contains(c));
    Console.WriteLine("Your total number of vowels is: {0}", total);
    Console.ReadLine();
}

Solution 2

Since Reed has answered your question, I will offer you another way to implement this. You can eliminate your loop by using LINQ and lambda expressions:

string sentence = "The quick brown fox jumps over the lazy dog.";
int vowelCount = sentence.Count(c => "aeiou".Contains(Char.ToLower(c)));

If you don't understand this bit of code, I'd highly recommend looking up LINQ and Lambda Expressions in C#. There are many instances that you can make your code more concise by eliminating loops in this fashion.

In essence, this code is saying "count every character in the sentence that is contained within the string "aeiou". "

Solution 3

That's because your if statement is always true, you need to compare the character at sentence[i], and see if it is a vowel, instead of seeing if the sentence contains a vowel.

Solution 4

Or with linq.

static void Main()
    {
        int total = 0;

        Console.WriteLine("Enter a Sentence");
        string sentence = Console.ReadLine().ToLower();
        char[] vowels = { 'a', 'e', 'i', 'o', 'u' };

        total = sentence.Count(x => vowels.Contains(x));

        Console.WriteLine("Your total number of vowels is: {0}", total);

        Console.ReadLine();
    }

Solution 5

You were checking to see if your whole sentence contained vowels for every iteration of your loop, which is why your total was simply the number of characters in your sentence string.

foreach(char ch in sentence.ToLower())
    if("aeiou".Contains(ch))
        total++;

Better yet use a regular expression. edit You'd only want to use a regex for something a little more complex than matching vowels.

using System.Text.RegularExpressions;
...
int total = Regex.Matches(sentence, @"[AEIOUaeiou]").Count;

EDIT Just for completeness the fastest/most efficient (if you were to do this on a ~million strings) solution. If performance wasn't a concern I'd use Linq for its brevity.

public static HashSet<char> SVowels = new HashSet<char>{'a', 'e', 'i', 'o', 'u'};
public static int VowelsFor(string s) {
    int total = 0;
    foreach(char c in s)
        if(SVowels.Contains(c))
            total++;
    return total;
}

View more solutions

Comments

[iCantSeeSharp]

Maybe if you make this case insensitive could be a bit better. And also provide a link to LINQ for starters.

[CBredlow]

That looks like an overly complex way to do it, and forgive me since it's been a while since I worked with linq, but wouldn't that just return 1?

[Phorden]

[i] is separating sentence into a char array to check per character instead of the whole sentence?

[Reed Copsey]

@Phorden No - string has an indexer that returns a character without turning it into an array.

[Reed Copsey]

@Souvlaki It's already case insensitive - the original post used ToLower, and I kept that ;)

[Tyler]

No, it returns the number of vowels in the string.

[Phorden]

@Reed Copsey okay, that makes sense. Thanks.

[Tyler]

@ReedCopsey oops didn't realize he called it when he read it. Will fix

[Chase Florell]

int total = sentence.Count(c => vowels.Contains(c)); could be int total = sentence.ToLower().Count(c => vowels.Contains(c));`

[Reed Copsey]

@ChaseFlorell No need - string sentence = Console.ReadLine().ToLower(); "sentence" is already guaranteed lower case at that point.

[Chase Florell]

Good call, I was testing it without reading line. I just used var sentence = "TESTING"; and it came back 0.

[Louis Ricci]

A regex is just as simple as linq -- int total = Regex.Matches(sentence, @"[AEIOUaeiou]").Count; --

[Reed Copsey]

@LastCoder You sure it's more efficient? It's easy enough, but regex compilation is probably going to be slower. (I do agree it's a good alternative, and did vote up your answer ;) )

[fatsmcgee]

Thanks. Have edited the comment to properly account for that.

[Reed Copsey]

@LastCoder Just wired up a quick timing - the RegEx is about 5x slower than LINQ - probably doesn't matter (both are VERY fast in this case), but just an FYI. (If I precompile the regex, it speeds up subsequent matches, but still is slower...)

[Louis Ricci]

@ReedCopsey - Just for curiosity's sake, a foreach + Contains loop is actually the fastest, Regex is slower than Linq. ideone.com/OXYc3U In hindsight it makes sense since Regex.Matches is generating match objects.

[Reed Copsey]

@LastCoder Did you try one with my code? I was using HashSet for the Contains check - my timings were far lower for mine, but similar for regex...

[Louis Ricci]

@ReedCopsey - Yes the HashSet<char> improves performance significantly. Changing the foreach loop over to one as well yields a similar gain. Keeping foreach + contains about 25% faster than linq Count ideone.com/AfZuZ6 which is curious since they are both performing the same thing, I guess linq just has a little more overhead.

[Reed Copsey]

@LastCoder LINQ typically has a slight overhead vs unrolling the loops, so that's pretty much expected. There's more allocations, so enough LINQ statements will often trigger a GC collection (or many), which is typically the difference.

[puretppc]

char[] vowels = { 'a', 'e', 'i', 'o', 'u' }; This works too.

[Nad]

superb explanation Sir..!!