Report on user-input chacters by their type I'm a first year student at a college that focuses heavily on self learning (SCRUM) without any classical lessons. Because of this, I basically learned everything I know from websites like this. I don't want to get any bad habits or wrong understandings. So, is this code any good? I'm not looking for optimization (wouldn't mind any tips though ;) ) since I'll be learning that through time, but rather if the general structure and my way of thinking is right.

The purpose of my code is to ask the user for a string of characters of his liking (for example a sentence) and displays the amount of vowels, consonants, numbers and other symbols it contains.

static void Main(string[] args)
{
    string repeat;
    //do while loop for if the user wants to run the program again
    do
    {
        //asigns variables
        string text;
        int vowels, consonants, numbers, otherSymbols;
        var hsVowels = new HashSet&lt;char&gt; { 'a', 'e', 'i', 'o', 'u' };
        var hsConsonants = new HashSet&lt;char&gt; { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z'};
        var hsNumbers = new HashSet&lt;char&gt; { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

        //asks for input
        Console.WriteLine("Input anything and the program wil tell you how many vowels, consonants, numbers and other symbols you gave.");
        text = Console.ReadLine().ToLower();

        //calculates
        vowels = text.Count(c =&gt; hsVowels.Contains(c));
        consonants = text.Count(c =&gt; hsConsonants.Contains(c));
        numbers = text.Count(c =&gt; hsNumbers.Contains(c));
        otherSymbols = text.Length - (vowels + consonants + numbers);

        //shows the result
        Console.WriteLine("Your input has {0} vowels, {1} consonants, {2} numbers and {3} other Symbols.", vowels, consonants, numbers, otherSymbols);

        //asks if the user wants to run the program again
        Console.WriteLine("Would you like to try again? (yes/no)");
        repeat = Console.ReadLine();
        //tests if the users input was valid (yes/no)
        while (!(repeat.ToLower().Contains("yes") || repeat.ToLower().Contains("no")))
        {
            Console.WriteLine(@"Invalid input. Please answer ""yes"" or ""no"" .");
            repeat = Console.ReadLine();
        }
    } while (repeat.ToLower().Contains("yes"));
}


Reposted from Stack Overflow to Stack Exchange as recommended by sellotape
Answering your main question: 


  So, is this code any good?


Your code is good in the sense that it works as expected in the context of the English alphabet. That's a good start and for a small program like this we could live with that.

But there are a couple of things to be done better.


    Console.WriteLine("Input anything and the program wil tell you how many vowels, consonants, numbers and other symbols you gave.");
    text = Console.ReadLine().ToLower();



Your prompts are a little too descriptive. Make them shorter and more "demanding" like:

Console.Write("Enter the text: ");
text = Console.ReadLine().ToLower();


Notice the use of Console.Write(...) instead of Console.WriteLine(...). The effect is, that the user enter the text on the same line after the prompt, which is common practice in the console.

If you want to inform the user of the meaning by the program, then provide a explanation once in the beginning - before to loop.




  var hsVowels = new HashSet&lt;char&gt; { 'a', 'e', 'i', 'o', 'u' };
  var hsConsonants = new HashSet&lt;char&gt; { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z' };
  var hsNumbers = new HashSet&lt;char&gt; { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };



These variables are constant and do not change per loop, so no need to instantiate them in every loop. Instead place them in the beginning of the method:

  string repeat;

  var hsVowels = new HashSet&lt;char&gt; { 'a', 'e', 'i', 'o', 'u' };
  var hsConsonants = new HashSet&lt;char&gt; { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z' };
  var hsNumbers = new HashSet&lt;char&gt; { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };

  do
  {
    //asks for input





    int vowels, consonants, numbers, otherSymbols;



Declaring variables like this is allowed, but considered bad practice. Instead use one line per variable:

    int vowels = 0;
    int consonants = 0;
    int numbers = 0;
    int otherSymbols = 0;


It's easier to find them and hence understand the code - especially for reviewers.




    //calculates
    vowels = text.Count(c =&gt; hsVowels.Contains(c));
    consonants = text.Count(c =&gt; hsConsonants.Contains(c));
    numbers = text.Count(c =&gt; hsNumbers.Contains(c));
    otherSymbols = text.Length - (vowels + consonants + numbers);



Here you use the LINQ extension Count() which is usable for many things, but here it is rather inefficient in that calling text.Count(...) for every char type will cause the text to be parsed three times. For a small text it will probably be OK, but for larger texts it will slow down the process significantly. Instead a single foreach-loop could do the same in just one iteration:

    foreach (char ch in text)
    {
      if (hsVowels.Contains(ch))
        vowels++;
      else if (hsConsonants.Contains(ch))
        consonants++;
      else if (hsNumbers.Contains(ch))
        numbers++;
      else
        otherSymbols++;
    }


It looks maybe not so smart and modern but is a lot more efficient. 




    Console.WriteLine("Your input has {0} vowels, {1} consonants, {2} numbers and {3} other Symbols.", vowels, consonants, numbers, otherSymbols);



Printing the result on one line is not very readable. A tabular form will be more clear:

    Console.WriteLine("Result: ");
    Console.WriteLine($"Original Text: {text}");
    Console.WriteLine($"Vowels:        {vowels,15}");
    Console.WriteLine($"Consonants:    {consonants,15}");
    Console.WriteLine($"Numbers:       {numbers,15}");
    Console.WriteLine($"OtherSymbols:  {otherSymbols,15}");





    Console.WriteLine("Would you like to try again? (yes/no)");
    repeat = Console.ReadLine();
    //tests if the users input was valid (yes/no)
    while (!(repeat.ToLower().Contains("yes") || repeat.ToLower().Contains("no")))
    {



Here, the user is forced to type "yes" or "no". "y" or "n" would do the trick:

Console.Write("Would you like to try again? (Y/N): ");


you use repeat.ToLower() every time you use repeat so why not lower it once:

repeat = Console.ReadLine()?.ToLower();




When it comes to the overall structure you should think about splitting the code into meaningful methods. The program has three major parts:


User input
The calculation
Display of the result


and because you allow the user to enter more than one text it must run in a loop like:

  string textInput;

  while ((textInput = GetUserInput()) != null)
  {
    TextInfo info = CountElements(textInput);
    PrintTextInfo(textInput, info);
  }


This is easy to understand, and each functionality of the program can be written and maintained separately:

private string GetUserInput()
{
  Console.Clear();
  Console.Write("Enter some Text [To exit press: Enter]: ");
  string text = Console.ReadLine();
  if (string.IsNullOrWhiteSpace(text))
    return null;
  return text;
}



private void PrintTextInfo(string textInput, TextInfo info)
{
  Console.WriteLine("Result: ");
  Console.WriteLine($"Original Text: {textInput}");
  Console.WriteLine($"Vowels:        {info.Vowels,15}");
  Console.WriteLine($"Consonants:    {info.Consonants,15}");
  Console.WriteLine($"Numbers:       {info.Numbers,15}");
  Console.WriteLine($"OtherSymbols:  {info.OtherSymbols,15}");
  Console.WriteLine("Press Enter to Continue...");
  Console.ReadLine();
}


For the TextInfo info = CountElements(textInput); you could write a text analyzer class that could be instantiated like:

  CharTypeAnalyzer analyzer = new CharTypeAnalyzer("aeiouAEIOU", "bcdfghijklmnpqrstvwxyzBCDFGHIJKLMNPQRSTVWXYZ");


with the definition of vowels and consonants as string arguments. This will make it more reusable for other languages than English (the use is of course limited to languages where the terms "vowel" and "consonant" are meaningful).

Using this class will change the main method to:

static void Main()
{
  CharTypeAnalyzer analyzer = new CharTypeAnalyzer("aeiouAEIOU", "bcdfghijklmnpqrstvwxyzBCDFGHIJKLMNPQRSTVWXYZ");

  string textInput;

  while ((textInput = GetUserInput()) != null)
  {
    TextInfo info = analyzer.Analyze(textInput);
    PrintTextInfo(textInput, info);
  }
}


where TextInfo is defined as:

public struct TextInfo
{
  public int Vowels;
  public int Consonants;
  public int Numbers;
  public int OtherSymbols;
}


and CharTypeAnalyzer as:

public class CharTypeAnalyzer
{
  string _vowels;
  string _consonants;

  public CharTypeAnalyzer(string vowels, string consonants)
  {
    _vowels = vowels;
    _consonants = consonants;
  }

  public TextInfo Analyze(string textInput)
  {
    TextInfo info = new TextInfo();

    foreach (char ch in textInput)
    {
      if (IsVowel(ch))
        info.Vowels++;
      else if (IsConsonant(ch))
        info.Consonants++;
      else if (IsDigit(ch))
        info.Numbers++;
      else
        info.OtherSymbols++;
    }

    return info;
  }

  private bool IsVowel(char ch)
  {
    return _vowels.Contains(ch);
  }

  private bool IsConsonant(char ch)
  {
    return _consonants.Contains(ch);
  }

  private bool IsDigit(char ch)
  {
    return char.IsDigit(ch);
  }
}


In this way the code is easy to review and maintain, and you can easily debug and test each part separately. 

It can be done in many other ways, but the essential messages are: 


Keep things simple
Split the code into meaningful parts - methods and classes