Text menu and validation of the chosen item

Question

I am writing a c# console app that will take a user input from a list of selected options and do things based on the selected option. I have it working right now but just looking over it there seems that I should be able to shorten the code with other methods and make it perform a bit faster and more efficient but for some reason I just can't see it. Am I missing something or is this really the way to be doing this check?

static void Main(string[] arguments)
{
 if (arguments.Length == 0)
 {
 Console.WriteLine("No run mode entered. Usage: JobSightMaintenance <FirstRun | NightlyMaintenance>");
 Console.WriteLine("Please select one of the following options to run:");
 printOptions();
 string userInput;
 bool isValidInput = true;
 do
 {
 userInput = Console.ReadLine();
 int selectedOption;
 try
 {
 selectedOption = int.Parse(userInput);
 }
 catch (Exception ex)
 {
 Console.WriteLine("Invalid Option, please try again");
 printOptions();
 isValidInput = false;
 break;
 }
 switch (selectedOption)
 {
 case 1:
 isValidInput = true;
 break;
 case 2:
 isValidInput = true;
 break;
 default:
 Console.WriteLine("Invalid Option, please try again");
 printOptions();
 isValidInput = false;
 break;
 }
 }
 while (!isValidInput);
 }
 if (Debugger.IsAttached) { Console.ReadKey(); }
}
private static void printOptions()
{
 Console.WriteLine("1. Run Intial Data Import");
 Console.WriteLine("2. Nightly Maintenance");
}

• Denis: You should be using int.TryParse() instead of try/catch block

Answer 1

Try to keep you Main method simple. It should the core logic of your application. The whole "get option and try again if failed" doesn't really belong in the Main method, and should be extracted into a separated method, not just the print options part.

static void Main(string[] arguments)
{
 if (arguments.Length == 0)
 {
 Console.WriteLine("No run mode entered. Usage: JobSightMaintenance <FirstRun | NightlyMaintenance>");
 Console.WriteLine("Please select one of the following options to run:");
 switch (GetUserOption())
 {
 case 1: 
 // Run Intial Data Import
 break;
 case 2:
 // Nightly Maintenance
 break;
 default:
 // Keep GetUserOption result in sync with this switch
 throw new InvalidOperationException();
 }
 }
 if (Debugger.IsAttached)
 {
 Console.ReadKey();
 }
}
static int GetUserOption()
{
 var options = new Dictionary<int, string>();
 options[1] = "Run Intial Data Import";
 options[2] = "Nightly Maintenance";
 while (true)
 {
 foreach (var option in options)
 Console.WriteLine("{0}. {1}", option.Key, option.Value);
 var input = Console.ReadLine();
 var selectedOption = options.SingleOrDefault(x => x.Key.ToString() == input);
 if (default(KeyValuePair<int, string>).Equals(selectedOption))
 {
 Console.WriteLine("Invalid Option, please try again");
 continue;
 }
 return selectedOption.Key;
 }
}

---

• Do not use int.Parse when you expected it to fail. Use int.TryParse instead, like this :

  while (true)
  {
   var input = Console.ReadLine();
   int option;
   if (!int.TryParse(input, out option))
   {
   Console.WriteLine("invalid option");
   continue;
   }
   // do something with option
  }
  

---

EDIT: You can also replace the magic numbers with constants to reduce the chance of messing up :

const int InitialDataImportKey = 1;
const int NightlyMaintenanceKey = 2;
static void Main(string[] arguments)
{
 // ...
 switch (GetUserOption())
 {
 case InitialDataImportKey:
 // ...
 break;
 case NightlyMaintenanceKey:
 // ...
 break;
 }
 // ...
}
static int GetUserOption()
{
 var options = new Dictionary<int, string>();
 options[InitialDataImportKey] = "Run Intial Data Import";
 options[NightlyMaintenanceKey] = "Nightly Maintenance";
 // ...
}

Answer 2

Let’s suppose that perfectness is our goal :)

 static void Main(string[] args)
 {
 new Menu("Please select one of the following options to run:")
 .Option("Run Initial Data Import", () => Console.WriteLine("Importing..."))
 .Option("Nightly Maintenance", () => Console.WriteLine("I am the terror (that flaps in the night)!"))
 .Option("Exit")
 .Execute();
 }

Where:

public class Menu 
{
 public Menu(string prompt = "Please choose:")
 : this(prompt, Enumerable.Empty<Option>())
 {
 }
 Menu(string prompt, IEnumerable<Option> options)
 {
 Prompt = prompt;
 Options = options;
 }
 string Prompt { get; }
 IEnumerable<Option> Options { get; }
 public Menu Option(string name, Action action = null) =>
 new Menu(Prompt, Options.Concat(new[] { new Option(name, action ?? (() => {})) }));
 public override string ToString() =>
 Prompt + "\n\r" +
 string.Join("\n\r", Options.Select((o, i) => $"{i+1}. {o.Name}"));
 public void Execute() => Execute(Console.Out, Console.In);
 public void Execute(TextWriter writer, TextReader reader)
 {
 writer.WriteLine(this);
 int i;
 if (int.TryParse(reader.ReadLine(), out i) && --i >= 0 && i < Options.Count())
 Options.ElementAt(i).Execute();
 else
 Execute(writer, reader);
 }
}

And

class Option
{
 public Option(string name, Action action)
 {
 Name = name;
 Execute = action;
 }
 public string Name { get; }
 public Action Execute { get; }
}

• Xiaoy312: Beautiful fluent syntax. However, there is one small thing that you could improve. Instead of creating a new menu object on every Option call, you can make Options a IList<Option> and have Option implemented as such { Options.Add(new Option(name, action ?? (() => { }))); /* new-line */ return this; }.
• Dmitry Nogin: @Xiaoy312 Thanks. Actually, I keep such kind of things immutable intentionally, as functional programming recommends. It is a way more natural (expected) for fluent API and allows to capture result for latter reuse at any moment: we might construct Menu once and feel free to share it for reuse. There were a lot of arguments about functional programming (over imperative), but it never hurts to keep amount of moving parts (mutable) as low as possible.
• Matthew Verstraete: @DmitryNogin Thank you for this, it appears beyond my abilities right now but I will be studying this to figure out how it all works.

Answer 3

I would use

int selectedOption = int.Parse(Console.ReadLine());

Wrap this piece of code into one method

private static void invalidOption()
{
 Console.WriteLine("Invalid Option, please try again");
 printOptions();
 isValidInput = false;
}

Then, I would use a list of options

List<string> optionsList = new List<string>();

I'd add the options to that string

optionsList.add("Run Intial Data Import");
optionsList.add("Nightly Maintenance");

I'd change the printOptions() method this way:

for(int i = 0; i < optionsList.size(); i++)
 Console.WriteLine(i + ". " + optionsList[i]);

Finally, change the switch statement as follows:

if ((selectedOption < optionsList.count) && (selectedOption >= 0))
 isValidInput = true;
else
 invalidOption();

In this way you want to add a new option, you just need to add the string to the list, and you don't have to worry to change the switch statement and to count how many options are available

• Xiaoy312: You can select the item with optionsList[i], no need to use ElementAt. Also, size() doesnt exist in C#, it should be .Count.
• alessalessio: Thanks, i ll edit the answer. I wanted just to point out a way of grouping code and create reusable methods, rather than giving a c# lesson.
• Xiaoy312: You need to validate the input better, as 0 or any negative number would be a valid input.
• Matthew Verstraete: I don't think I use int selectedOption = int.Parse(Console.ReadLine());. That would require me to pull it out of the try..catch and any invalid input (like a string) would crash the program. Unless your also suggesting running the entire program in a single try..catch which I don't think is a good thing to do.