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\$
9
  • \$\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, 2016 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, 2016 at 17:56
  • \$\begingroup\$ I like how you've abstracted away the user interface to helper methods. \$\endgroup\$ May 24, 2016 at 12:33
  • 1
    \$\begingroup\$ I am serious. Though it is a little weird that those methods are not void returning. \$\endgroup\$ May 24, 2016 at 14:17
  • 1
    \$\begingroup\$ Just remove the returns and change the return type to void. \$\endgroup\$ May 24, 2016 at 15:30

1 Answer 1

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\$
11
  • \$\begingroup\$ What do you mean count isn't intentionally used? \$\endgroup\$
    – 13aal
    May 23, 2016 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, 2016 at 17:42
  • 1
    \$\begingroup\$ that looks like my answer on the previous question --> codereview.stackexchange.com/a/128778/18427 \$\endgroup\$
    – Malachi
    May 23, 2016 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, 2016 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, 2016 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.