10
\$\begingroup\$

I'm just learning C# and wrote a little program to check if a word contains vowels. It's fairly simple and not to exciting but I would like to know if there's easier ways to do this:

using System;

namespace check_vowels
{
    class Program
    {
        static void Main(string[] args)
        {
            string word;

            Console.WriteLine("We will check if your string contains any vowels or not.");
            Console.Write("Enter a word: ");
            word = Console.ReadLine();
            if (word.ToLower().Contains("a") || (word.ToLower().Contains("e")))
            {
                Console.WriteLine("Your word contains the vowel 'A' or the Vowel 'E', or both.");
            }
            else if ((word.ToLower().Contains("i")) || (word.ToLower().Contains("o")))
            {
                Console.WriteLine("Your word contains the vowel 'I', or 'O', or both.");
            }
            else if (word.ToLower().Contains("u"))
            {
                Console.WriteLine("Your word contains the vowel 'U'.");
            }
            else
            {
                Console.WriteLine("Your word contains no vowels.");
            }

            Console.ReadLine();
        }
    }
}

What can I do better? What did I do well? How can I shorten this?

\$\endgroup\$
  • 1
    \$\begingroup\$ Man, you C# guys are awesome, thank you all for the answers and the views. \$\endgroup\$ – 13aal May 19 '16 at 17:30
  • \$\begingroup\$ In going to rewrite this and implement everyone's valid points, thank you all for the feedback it was greatly appreciated and I look forward to the feedback on the next version of this program \$\endgroup\$ – 13aal May 23 '16 at 0:59
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success May 23 '16 at 17:32
14
\$\begingroup\$

You could potentially do:

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

foreach(var vowel in vowels)
     Console.WriteLine("Your phrase contains a vowel of {0}", vowel);

if(!vowels.Any())
    Console.WriteLine("No vowels have been detected.");

So you understand the code a bit:

  • Where() : Will test each character to see if it is indeed a vowel or not.
  • Distinct() : Will remove duplicate vowels found.

Then you loop through the vowels stating what exist inside, then the if will simply return when loop is empty.

\$\endgroup\$
  • 1
    \$\begingroup\$ Note you can also do Console.Readline().ToLower().ToCharArray().Intersect("aeiou".ToCharArray()); \$\endgroup\$ – Ben Aaronson May 19 '16 at 16:16
  • 1
    \$\begingroup\$ This is pretty cool, I'm really liking C# so far. \$\endgroup\$ – 13aal May 19 '16 at 16:18
  • \$\begingroup\$ I suggest .ForEach for consistency with FP: msdn.microsoft.com/en-us/library/bwabdf9z(v=vs.110).aspx \$\endgroup\$ – Caridorc May 19 '16 at 16:24
  • \$\begingroup\$ @Ben Aaronson I would create a separate variable vca="aeiou".ToCharArray(); so that conversion isn't done over and over for each input line (The compiler might not optimize that out). \$\endgroup\$ – FredK May 19 '16 at 16:47
  • 6
    \$\begingroup\$ The ToCharArray is unnecessary; a string is already IEnumerable<char>. \$\endgroup\$ – Eric Lippert May 19 '16 at 20:25
13
\$\begingroup\$

Let's look at a problem with your program than another answer touched on (talking about Welsh). In Welsh,

cwm

means "valley", and is pronounced "coom". (Often rendered in English as coombe.) w is a vowel.

Suppose I am French and I type in the French word for "tea":

thé

Does it contain a vowel? I sure think it does. Does your program classify it as containing a vowel?

I am Norwegian, so I type in:

Vær så

Does it contain a vowel? I sure think it does. Does your program?

I'm Turkish, so I type in:

ışık

And so on.

This last one is particularly interesting. Turkish has capital I with dot, capital I with no dot, small I with dot, small I without dot. When you lower-case capital I without dot in English, you get small I without dot. Not in Turkish! In Turkish the small version of I is ı, not i. So your program would say that "I" has no vowels when the program is run in Turkey!

You're a beginner programmer; now is a good time to realize that if you become a professional, you are going to have to deal with writing programs that work everywhere in the world, and that means thinking about localization issues like this. Does it really matter in your practice program? Of course not. But believe me, in industry these sorts of issues are very real.

\$\endgroup\$
  • 3
    \$\begingroup\$ @13aal: Point being: ToLower/ToUpper are not reliable ways to canonicalize a string. Lowercasing gives different results in different parts of the world. Failure to understand this fact has caused serious real-world security defects; comparing ToUpper("mıcrosoft.com") to "MICROSOFT.COM" succeeds in Turkey, but mıcrosoft.com might be a site owned by hackers, not Microsoft. \$\endgroup\$ – Eric Lippert May 19 '16 at 20:14
  • 1
    \$\begingroup\$ Those are really good points, to deal with Culture Variants. \$\endgroup\$ – Greg May 20 '16 at 13:21
  • 2
    \$\begingroup\$ @13aal This is a surprisingly hard problem in general. Consider how Hebrew represents vowels, for instance. You will learn a lot about different cultures and their writing systems! But yes that basic approach is a good way to start. \$\endgroup\$ – Eric Lippert May 23 '16 at 1:14
  • 1
    \$\begingroup\$ @13aal: If you already know some Arabic then you have a head start in understanding these sorts of issues. Many developers are shockingly ignorant of even the basics of other culture's writing systems. I once had a developer ask me how to translate between the "English letter C" and the "Chinese letter C" and it took quite some time to convince them that Chinese does not actually have an alphabet the way that English does. \$\endgroup\$ – Eric Lippert May 23 '16 at 17:49
  • 3
    \$\begingroup\$ @200_success: Indeed, I always recommend that beginner programers start by writing a specification for every method, even if that specification is a single sentence. It clarifies your thinking before you even begin writing the code. If you have a specification of even a single sentence then you should be able to deduce the method signature, you should be able to think about whether it is pure or impure, you should be able to think of some test cases, and so on. \$\endgroup\$ – Eric Lippert May 23 '16 at 17:51
6
\$\begingroup\$

Although this is just a small example, there are some lessons that assume greater importance when you are writing bigger programs. The two guidelines I wish to promote are:

Separate the definitions from the logic

In your function, the knowledge of which characters are classed as vowels is buried deep inside. It's often better to use a data structure (perhaps just a list or an array) to hold that knowledge, and then use that from your function. There are two benefits:

  1. Somebody reading your code for the first time can see, early in the file, the data you'll be using. They will find it much easier to add missing vowels such as if there's just one obvious place to do so.
  2. When you decide that you want to treat w and y (and ŵ) as vowels when the input is in Welsh, but not for English input, you can pass a different data table into the function depending on the language.

    As an aside, we should note that toLower is not exactly equivalent to a case-insensitive comparison in many locales.

Separate processing from I/O

It's hard to unit-test your code, because the reading and writing are done in the same function as the processing. If you write the processing as a function, you can write a main() that repeatedly tests that function with known inputs and expected outputs.

(You might be able to tell that I'm not fluent in C#; that's why I'm not providing code samples to back up my advice)

\$\endgroup\$
5
\$\begingroup\$

As a general note, I'd say that code like this is great for learning and getting comfortable with a language, IDE, etc. But it's unlikely to get reviews which are very helpful.

If you want to write code to be reviewed, I'd suggest you start off with a clear set of requirements that makes sense, then try to implement them. For example, a bowling scorer, a Roman numerals translator, or a 2-player Tic-Tac-Toe game. That way we'll be able to review how well your code achieved your aim.


As for your code, there's not much to point out. Some things:

  • Your messages are a bit inconsistent. You say "or the Vowel 'E'" but just "or 'O'". And that "Vowel" also has a capital V.

  • A user probably won't understand that if a message says "A or E or both", then it may also have an I, O or U. Likewise if they get told "U", they won't know that means there's no A,E,I or Os. This is weird, but presumably intentional (which goes back to why it's hard to usefully review code like this)

  • Your namespace is check_vowels. By convention, namespace parts are PascalCased, so that should be CheckVowels

  • You can remove the line where you declare word and just use var word = Console.ReadLine();

  • If you do var word = Console.ReadLine().ToLower();, you won't need to keep calling ToLower later

\$\endgroup\$
  • 1
    \$\begingroup\$ I understand where you're coming from, I'm just starting, this is literally my second day, and I've been writing little scripts like this to just figure out what I was doing. This one seemed a little sloppier then my other, so I was just curious if there where easier ways to do this. \$\endgroup\$ – 13aal May 19 '16 at 16:19
  • 5
    \$\begingroup\$ @13aal Yep fair enough, it's certainly not a bad question or anything. \$\endgroup\$ – Ben Aaronson May 19 '16 at 16:20
2
\$\begingroup\$

On some comparisons you check for 2 vowels but for U you only check for one, I am thinking that you would want to check for each one separately.

Here is the version that I created while everyone was answering with wonderful answers using Linq and built-in String Methods, I don't think I used anything they haven't already mentioned.

Console.WriteLine("We will check if your string contains any vowels or not.");
Console.Write("Enter a word: ");
var vowelsInWord = Console.ReadLine().ToLower().Intersect("aeiou");
if (!vowelsInWord.Any())
{
    Console.WriteLine("There are no vowels in this word, how do you pronounce that?");
}
else
{
    foreach (var vowel in vowelsInWord)
    {
        Console.WriteLine($"Your word contains the vowel '{vowel}'");
    }
}

Sorry, I do use something that the other answers haven't mentioned, and it has to do with the output string.

Console.WriteLine($"Your word contains the vowel '{vowel}'");

It is called string interpolation, and it is currently available in with the current version of Visual Studio 2015 and C# 6.0, because that is what I am using and it works.

This allows you to use the variable name inside of the curly braces ({}), it will return the string representation of whatever you have inside the braces as long as you precede the string with a ($)


I also chose to use the Intersect method as well rather than to use Where.

Where and Intersect are similar except for the fact that Intersect will return a list of distinct values, and Where will return duplicates if there is more than one matching value in either list.

Depending on what you are wanting to do, it will make a difference as to which one that you use. I didn't want the program to duplicate the phrase for each instance of the vowel in the word, that is why I chose Intersect instead of Where.

\$\endgroup\$
  • 1
    \$\begingroup\$ Awesome, I didn't know that C# has string interpolation, thanks for the answer! \$\endgroup\$ – 13aal May 19 '16 at 17:14

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.