# Text menu and validation of the chosen item

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
{
int selectedOption;

try
{
selectedOption = int.Parse(userInput);
}
catch (Exception ex)
{
printOptions();
isValidInput = false;
break;
}

switch (selectedOption)
{
case 1:
isValidInput = true;
break;

case 2:
isValidInput = true;
break;

default:
printOptions();
isValidInput = false;
break;
}
}
while (!isValidInput);
}

}

private static void printOptions()
{
Console.WriteLine("1. Run Intial Data Import");
Console.WriteLine("2. Nightly Maintenance");
}

• You should be using int.TryParse() instead of try/catch block May 27 '16 at 19:23

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)
{
}
}

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 selectedOption = options.SingleOrDefault(x => x.Key.ToString() == input);

if (default(KeyValuePair<int, string>).Equals(selectedOption))
{
continue;
}

return selectedOption.Key;
}
}


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

while (true)
{

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";

// ...
}


Let’s suppose that perfectness is our goal :)

    static void Main(string[] args)
{
.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
{
: this(prompt, Enumerable.Empty<Option>())
{
}

{
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);

{
writer.WriteLine(this);
int i;
if (int.TryParse(reader.ReadLine(), out i) && --i >= 0 && i < Options.Count())
Options.ElementAt(i).Execute();
else
}
}


And

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

public string Name { get; }
public Action Execute { get; }
}

• 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; }. May 27 '16 at 20:19
• @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. May 27 '16 at 20:49
• @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. May 28 '16 at 2:32

I would use

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


Wrap this piece of code into one method

private static void invalidOption()
{
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");


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

• You can select the item with optionsList[i], no need to use ElementAt. Also, size() doesnt exist in C#, it should be .Count. May 27 '16 at 16:07
• 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. May 27 '16 at 16:09
• You need to validate the input better, as 0 or any negative number would be a valid input. May 27 '16 at 16:10
• 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. May 27 '16 at 17:44