2
\$\begingroup\$

A few days ago I posted a little program that would check for vowels within a given word. I have rewritten this program, it can only find vowels in the English language still but, it will now tell you each vowel that is contained in your word, along with a count of how many vowels there are total, it's still not very exciting but I would like some more feedback on what I've done, anything will help:

using System;
using System.Linq;

namespace CheckForVowels
{
    class Vowels
    {
        static string say(string input)
        {
            Console.ForegroundColor = ConsoleColor.Green;
            Console.WriteLine(input);
            return input;
        }
        static string prompt(string input)
        {
            Console.ForegroundColor = ConsoleColor.Cyan;
            Console.Write($"{input}: ");
            return input;
        }
        static void Main(string[] args)
        {
            say("We will be checking your input to see if it contains vowels or not");
            prompt("Enter a word");

            var count = 0;
            var vowels = Console.ReadLine()
                .Where(c => "aeiouAEIOU".Contains(c))
                .Distinct();

            foreach (var vowel in vowels)
            {
                count += 1;
                say($"Your word contains vowel: {vowel}.");
            }

            say($"You have a total of {count} vowels in your word.");

                if (!vowels.Any())
                say($"Your word contains no vowels.");

        }

    }
}
\$\endgroup\$
  • \$\begingroup\$ have you tested this with no vowels in the word? I don't think your prompt and the readline will work the way you think it should work. when you use write you stay on the same line, so when you readline I think it will read the entire line with the prompt string \$\endgroup\$ – Malachi May 23 '16 at 17:47
  • \$\begingroup\$ @Malachi I have tested it without vowels, it seems to work for me. Are you getting a different issue..? \$\endgroup\$ – 13aal May 23 '16 at 17:56
  • \$\begingroup\$ I like how you've abstracted away the user interface to helper methods. \$\endgroup\$ – Eric Lippert May 24 '16 at 12:33
  • 1
    \$\begingroup\$ I am serious. Though it is a little weird that those methods are not void returning. \$\endgroup\$ – Eric Lippert May 24 '16 at 14:17
  • 1
    \$\begingroup\$ Just remove the returns and change the return type to void. \$\endgroup\$ – Eric Lippert May 24 '16 at 15:30
2
\$\begingroup\$

You should adhere to the following:

  • Capitalize method names.
  • You don't need count variable.
  • Don't specify a return type, then not utilize it.

Point (1) and (3) could be rectified:

protected static void WriteToConsole(string input)
{
     Console.ForegroundColor = ConsoleColor.Cyan;
     Console.WriteLine(input);
}

The application code could then be:

var vowels = Console.ReadLine().Where(c => "aeiouAEIOU".Contains(c));

if(vowels.Any())
{
     foreach(var vowel in vowels.Distinct())
         WriteToConsole(String.Format("Your sentence contains the following vowel {0}", vowel);

    WriteToConsole(String.Format("Your sentence contains {0} vowels", vowels.Count());
}
else
{
     WriteToConsole("Your sentence doesn't have any vowels.");
}

Also to point out, in the other question to avoid duplicate entries an uneeded iterations of repeated vowels, I called .Distinct. This alleviated that, but that would throw off your official count of how many vowels exist within the sentence. So I call Distinct on the loop rather than the initial input.

Also to avoid an unneeded count variable, I call .Count() on the IEnumerable, the feature exist so why not use it?

\$\endgroup\$
  • \$\begingroup\$ What do you mean count isn't intentionally used? \$\endgroup\$ – 13aal May 23 '16 at 17:35
  • \$\begingroup\$ @13aal I modified my answer, you don't need it. You can simply call vowels.Count() rather than create a variable to store the counted index. \$\endgroup\$ – Greg May 23 '16 at 17:42
  • 1
    \$\begingroup\$ that looks like my answer on the previous question --> codereview.stackexchange.com/a/128778/18427 \$\endgroup\$ – Malachi May 23 '16 at 17:45
  • 1
    \$\begingroup\$ @Malachi It is quite similar, I didn't think about it like that. Why did you choose the Intersect rather than Where? Yeah, it looks like he used a mix of both our answers. \$\endgroup\$ – Greg May 23 '16 at 18:12
  • 2
    \$\begingroup\$ Produces the set intersection of two sequences by using the default equality comparer to compare values. \$\endgroup\$ – Greg May 24 '16 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.