Best Practices For A Console Menu App

Question

I am quite fresh to c# and creating programs. I wrote a very basic program which allows you to go through menu selections via up and down arrowkeys and select your selection along with sending you to a sub menu and back.

I feel like it's kind of clunky and suspect there's definitely better practices to utilize but unsure of any really. For example, most of my classes have public methods but I hear that's bad practice, however I couldn't necessarily find a way around this?

Here's the code below. Feel free to criticize it all!

Keys to use would be up arrow, down arrow, and enter.

***Edit is because this was flagged as "code not working" Which I think is a mistake because I just copied and pasted everything here into visual studio and it ran just fine. I believe because I had a sentence saying "not working to what it's intended to do" was taken out of context as it was more so that the actual program isn't done not that it wont run...I wish there was a more specific reason as to why this was closed. It's just a simple code review.

Program

using System.IO;
using System.Collections;
using System.Collections.Generic;
using System.Net;
using System;
namespace EmployeeAndClientDatabase
 {
internal class Program
{
 static void Main(string[] args)
 {
 Employee employee = new Employee();
 Client client = new Client();
 Menu menu = new Menu();
 Console.WriteLine("Welcome to the FrontStar Worker & Client Database System");
 Console.WriteLine();
 Console.WriteLine("*********************************************************");
 Console.WriteLine(@"* * 
 * * 
 * Press enter to continue. * 
 * *
 * *
 ********************************************************");
 Console.ReadLine();
 Console.Clear();
 menu.Start(client, employee);
 }
}
}

IPerson

interface IPerson
{
string NameFirst { get; set; }
string NameLast { get; set; }
string IdNumber { get; set; }
string Month { get; set; }
string Day { get; set; }
string Year { get; set; } 
string Address { get; set; }
int Social { get; set; }
 }

Employee

 public class Employee : IPerson
{
List<Employee> employees = new List<Employee>();
public string NameFirst { get; set; }
public string NameLast { get; set; } 
public string IdNumber { get; set; }
public string Month { get; set; }
public string Day { get; set; }
public string Year { get; set; }
public string Address { get; set; }
public int Social { get; set; }
 public Employee()
{
}
public Employee(string first, string last, string id, string month, string day, string year, string address, int social)
{
 this.NameFirst = first;
 this.NameLast = last;
 this.IdNumber = "E:" + id;
 this.Month = month;
 this.Day = day;
 this.Year = year;
 this.Address = address;
 this.Social = social;
}
public void EmployeeMenu()
{
 Console.WriteLine("Employee Sub-Menu");
 Console.ReadLine();
 Console.Clear();
}
public void AddEmployee()
{
 string first;
 string last;
 string id;
 string month;
 string day;
 string year;
 string address;
 int social;
 Console.WriteLine("Test: adding employee");
 Console.Write("name: ");
 first = Console.ReadLine();
 Console.Write("Last: ");
 last = Console.ReadLine();
 Console.Write("ID: ");
 id = Console.ReadLine();
 Console.Write("DOB Month: ");
 month = Console.ReadLine();
 Console.Write("DOB Day: ");
 day = Console.ReadLine();
 Console.Write("DOB Year: ");
 year = Console.ReadLine();
 Console.Write("Address: ");
 address = Console.ReadLine();
 Console.Write("Social: ");
 social = Convert.ToInt32(Console.ReadLine());
 employees.Add(new Employee(first, last, id, month, day, year, address, social));
}
public void ViewEmployees()
{
 string list;
 list = employees[0].ToString();
 Console.WriteLine(list);
}
public override string ToString()
{
 return $"First Name: {NameFirst}\nLast Name: {NameLast}\nID: {IdNumber} \nDate Of Birth: {Month} / {Day} / {Year} \nAddress: {Address} \nSocial Security: {Social} ";
}
 }

Client

 public class Client : IPerson
{
List<Client> clients = new List<Client>();
public string NameFirst { get; set; }
public string NameLast { get; set; }
public string IdNumber { get; set; }
public string Month { get; set; }
public string Day { get; set; }
public string Year { get; set; }
public string Address { get; set; }
public int Social { get; set; }
public Client(string first, string last, string id, string month, string day, string year, string address, int social)
{
 this.NameFirst = first;
 this.NameLast = last;
 this.IdNumber = "C:" + id;
 this.Month = month;
 this.Day = day;
 this.Year = year;
 this.Address = address;
 this.Social = social;
}
public Client()
{
}
public void ClientMenu()
{
 Console.WriteLine("Client Sub-Menu");
 Console.ReadLine();
 Console.Clear();
}
public void AddClient()
{
 string first;
 string last;
 string id;
 string month;
 string day;
 string year;
 string address; 
 int social;
 Console.WriteLine("Test: adding Client");
 Console.Write("name: ");
 first = Console.ReadLine();
 Console.Write("Last: ");
 last = Console.ReadLine();
 Console.Write("ID: ");
 id = Console.ReadLine();
 Console.Write("DOB Month: ");
 month = Console.ReadLine();
 Console.Write("DOB Day: ");
 day = Console.ReadLine();
 Console.Write("DOB Year: ");
 year = Console.ReadLine();
 Console.Write("Address: ");
 address = Console.ReadLine();
 Console.Write("Social: ");
 social = Convert.ToInt32(Console.ReadLine());
 clients.Add(new Client(first, last, id, month, day, year, address, social));
}
 }

Menu

 public class Menu
 {
public void Start(Client client, Employee employee)
{
 bool exit = false;
 //string[] options = { "Employees Menu", "Clients Menu" };
 int selectedOption = 0;
 ConsoleKey key;
 
 //ConsoleKeyInfo key = Console.ReadKey();
 string marker = "*";
 
 do
 {
 Console.WriteLine("*****+-: Main Menu :-+*****");
 Console.WriteLine("___________________________");
 Console.WriteLine();
 Console.WriteLine("Please make a selection:");
 Console.WriteLine();
 Console.WriteLine($"{marker}Employee Menu");
 Console.WriteLine($"Client Menu");
 Console.WriteLine("Exit Program");
 
 do
 {
 key = Console.ReadKey(true).Key;
 
 if (key == ConsoleKey.DownArrow)
 {
 selectedOption++;
 
 }
 if(key == ConsoleKey.UpArrow)
 {
 selectedOption--;
 }
 
 if(key == ConsoleKey.Enter)
 {
 Console.Clear();
 break;
 }
 
 if (selectedOption < 0 || selectedOption >= 3)
 {
 if (selectedOption < 0)
 {
 selectedOption = 2;
 }
 if (selectedOption >= 3)
 {
 selectedOption = 0;
 }
 }
 switch (selectedOption)
 {
 case 0:
 Console.Clear();
 Console.WriteLine("*****+-: Main Menu :-+*****");
 Console.WriteLine("___________________________");
 Console.WriteLine();
 Console.WriteLine("Please make a selection:");
 Console.WriteLine();
 Console.WriteLine($"{marker}Employee Menu");
 Console.WriteLine($"Client Menu");
 Console.WriteLine($"Exit Program");
 break;
 case 1:
 Console.Clear();
 Console.WriteLine("*****+-: Main Menu :-+*****");
 Console.WriteLine("___________________________");
 Console.WriteLine();
 Console.WriteLine("Please make a selection:");
 Console.WriteLine();
 Console.WriteLine($"Employee Menu");
 Console.WriteLine($"{marker}Client Menu");
 Console.WriteLine($"Exit program");
 break;
 case 2:
 Console.Clear();
 Console.WriteLine("*****+-: Main Menu :-+*****");
 Console.WriteLine("___________________________");
 Console.WriteLine();
 Console.WriteLine("Please make a selection:");
 Console.WriteLine();
 Console.WriteLine($"Employee Menu");
 Console.WriteLine($"Client Menu");
 Console.WriteLine($"{marker}Exit Program");
 break;
 }
 
 } while (key != ConsoleKey.Enter);
 switch(selectedOption)
 {
 case 1:
 client.ClientMenu();
 break;
 case 0:
 employee.EmployeeMenu();
 break;
 case 2:
 exit = true;
 break;
 }
 selectedOption = 0;
 } while (exit == false);
}
public int Exit()
{
 return 0;
}
}

• Peter Csala: Are you interested review feedback only about menu? Or do you want to get feedback holistically?
• FloorSign: I say holistically would be best
• FloorSign: I guess to be more specific, feedback on how the logic is implemented? For example, at the main menu, you can "scroll" through the different options via the up and down arrow keys. But I feel that the way I'm making that work is kind of wonky. Or how once you select your option it takes you to a "sub menu" and then you can go back to main (basically the only option available) but I am not too sure if there is a better way to implement such feature that isn't as crude?
• Peter Csala: Did I cover everything? If so please consider to mark one of the posts as the answer.
• FloorSign: Yes, This definitely brought in some insight. Just trying to understand more of best practices and I believe this has covered it. Sorry for taking a while! And thank you so much!

Answer 1

The other answer focused on the data objects. This answer focuses on the menu.

Menu printing

Repetitive menu header

If you look at your code you can easily spot that the menu header is copy-pasted many times:

Console.WriteLine("*****+-: Main Menu :-+*****");
Console.WriteLine("___________________________");
Console.WriteLine();
Console.WriteLine("Please make a selection:");
Console.WriteLine();

You could extract this piece of code into a separate method and call it as many times as you need.

Multi-line text

In case of C# you can use multi-line text by using the @ before your string literal.
So, the above menu header could be rewritten to this:

Console.WriteLine(
@"*****+-: Main Menu :-+*****
___________________________
Please make a selection
");

Repetitive selected menu

If you look at your code you can easily spot that the selected menu highlighting is copy-pasted with a slight difference in each occasion

Console.WriteLine($"{marker}Employee Menu");
Console.WriteLine($"Client Menu");
Console.WriteLine("Exit Program");
...
Console.WriteLine($"Employee Menu");
Console.WriteLine($"{marker}Client Menu");
Console.WriteLine($"Exit program");
...

A better option could be to define the menu items in a single place and whenever you print the menu items you highlight the selected one:

private static void PrintMenu(List<MenuItem> items, MenuItem selectedItem)
{
 Console.Clear();
 Console.WriteLine(
@"*****+-: Main Menu :-+*****
___________________________
Please make a selection
");
 foreach (var item in items)
 {
 Console.Write(item == selectedItem ? "* " : " ");
 Console.WriteLine(item.Name);
 }
}

MenuItem

I would suggest to create a simple data structure to store the display name of the menu item and the to-be-executed action in case of selection.

For older C# versions you can easily do that via struct:

struct MenuItem
{
 public string Name {get; set;} 
 public Action Handler {get; set;}
 public static bool operator ==(MenuItem item1, MenuItem item2)
 => item1.Equals(item2);
 public static bool operator !=(MenuItem item1, MenuItem item2) 
 => !item1.Equals(item2);
}

For newer C# versions you can use record:

record MenuItem(string Name, Action Handler);

Menu Items

If you have defined the MenuItem as struct then the menu items can be defined like this:

var menu = new List<MenuItem>
{
 new() { Name = "Employee Menu", Handler = () => Console.WriteLine("employee")},
 new() { Name = "Client Menu", Handler = () => Console.WriteLine("client")},
 new() { Name = "Exit Program", Handler = () => Environment.Exit(0)},
};

if you have defined the MenuItem as record then:

var menu = new List<MenuItem>
{
 new("Employee Menu", () => Console.WriteLine("employee")),
 new("Client Menu", () => Console.WriteLine("client")),
 new("Exit Program", () => Environment.Exit(0)),
};

• I've used Console.WriteLine("xyz") for the sake of simplicity but you can easily wire here any arbitrary method call
• In case of Exit Program I've used Environment.Exit(0) to close the application immediately

Handling key press

The last remaining thing is to make the menu interactive. In your code you have two nested do-while loops. The good news is that you don't need both. The same behaviour can be achieved with a single one.

int selectedOption = 0;
int maxIndex = menu.Count-1;
do
{
 PrintMenu(menu, menu[selectedOption]);
 Action next = Console.ReadKey().Key switch
 {
 ConsoleKey.DownArrow => () =>
 {
 selectedOption = selectedOption == maxIndex ? 0 : selectedOption + 1;
 },
 ConsoleKey.UpArrow => () =>
 {
 selectedOption = selectedOption == 0 ? maxIndex : selectedOption - 1;
 },
 ConsoleKey.Enter => () =>
 {
 Console.Clear();
 menu[selectedOption].Handler();
 Console.ReadLine();
 }
 };
 next();
} while (true);

DownArrow

If the selected item is the last menu item then move to the first one otherwise move to the next one

UpArrow

If the selected item is the first menu item then move to the last one otherwise move to the previous one

Enter

1. Clear the screen
2. Execute the menu item's Handler method
3. Wait for any key press to move back to the main menu

You can replace the Console.ReadLine() to any arbitrary logic, like checking for Esc key

next()

This executes the method related to the associated key press. If you don't want to use switch expression then you can convert the logic back to simple if-else if statements:

do
{
 PrintMenu(menu, menu[selectedOption]);
 var key = Console.ReadKey().Key;
 if(key == ConsoleKey.DownArrow)
 {
 selectedOption = selectedOption == maxIndex ? 0 : selectedOption + 1;
 }
 else if(key == ConsoleKey.UpArrow)
 {
 selectedOption = selectedOption == 0 ? maxIndex : selectedOption - 1;
 }
 else if(key == ConsoleKey.Enter)
 {
 Console.Clear();
 menu[selectedOption].Handler();
 Console.ReadLine();
 }
} while (true);

---

For the sake of completeness here is the complete code

record MenuItem(string Name, Action Handler);
public static class Menu
{
 public static void Start()
 {
 var menu = new List<MenuItem>
 {
 new("Employee Menu", () => Console.WriteLine("employee")),
 new("Client Menu", () => Console.WriteLine("client")),
 new("Exit Program", () => Environment.Exit(0)),
 };
 int selectedOption = 0;
 int maxIndex = menu.Count-1;
 do
 {
 PrintMenu(menu, menu[selectedOption]);
 Action next = Console.ReadKey().Key switch
 {
 ConsoleKey.DownArrow => 
 () => selectedOption = selectedOption == maxIndex ? 0 : selectedOption +1,
 ConsoleKey.UpArrow => 
 () => selectedOption = selectedOption == 0 ? maxIndex : selectedOption -1,
 ConsoleKey.Enter => () =>
 {
 Console.Clear();
 menu[selectedOption].Handler();
 Console.ReadLine();
 }
 };
 next();
 } while (true);
 }
 private static void PrintMenu(List<MenuItem> items, MenuItem selectedItem)
 {
 Console.Clear();
 Console.WriteLine(
@"*****+-: Main Menu :-+*****
___________________________
Please make a selection
");
 foreach (var item in items)
 {
 Console.Write(item == selectedItem ? "* " : " ");
 Console.WriteLine(item.Name);
 }
 }
}

Answer 2

Disclaimer: I'll post two answers. This one will focus on the data objects the other one on the menu.

IPerson

This could be an abstract class rather than an interface. That would allow you to inherit the properties rather than defining them as many times as many classes implements the interface.

public abstract class Person
{
 public string NameFirst { get; set; }
 public string NameLast { get; set; }
 public abstract string IdNumber { get; set; }
 public string Month { get; set; }
 public string Day { get; set; }
 public string Year { get; set; } 
 public string Address { get; set; }
 public int Social { get; set; }
}

Since you have used E: and C: prefixes in the IdNumber that's why you have to define the IdNumber property as abstract inside the Person class.

public class Employee : Person
{
 private string idNumber;
 public override string IdNumber { get => idNumber; set => idNumber = $"E:{value}"; }
}

Naming

In case of Day, Month and Year the intended usage are unclear until you read the AddEmployee method's code. Prefixing the properties with BornAt/DateOfBirth/ whatever brings clarity.

You should also consider to have a single date structure instead of three string properties. You could use DateTime/DateTimeOffset or in newer .NET versions DateOnly.

Employee vs Employees

In the Employee class definition I did not include the employees private field. It is highly suggested to separate your class definition from its usage (whether it is just a single instance or multiple ones).

AddEmployee

The Convert.ToInt32 is error prone since it can throw FormatException if the provided parameter can't be converted to an int. Please prefer to use int.TryParse instead.

ViewEmployees

This method prints only the verify first employee if there is any. If there is no employee then it will throw IndexOutOfRangeException. I would suggest to use a simple foreach to print each employee.

foreach(var employee in employees)
{
 Console.WriteLine(employee);
}

ToString

In the future it might make sense to use reflection to dynamically generate your ToString method.

var sb = new StringBuilder();
foreach (var property in GetType().GetProperties())
{
 sb.AppendLine(property.Name + ": " + property.GetValue(this, null));
}
return sb.ToString();

If you wish to use different names than the properties' name then you can annotate the properties with the DisplayName attributes

public abstract class Person
{
 [DisplayName("First Name")]
 public string NameFirst { get; set; }
 ...
}

public override string ToString()
{
 var sb = new StringBuilder();
 foreach (var property in GetType().GetProperties())
 {
 var name = property.GetCustomAttribute<DisplayNameAttribute>()?.DisplayName ?? property.Name;
 sb.AppendLine(name + ": " + property.GetValue(this, null));
 }
 return sb.ToString();
}

---

The same suggestions could be applied to the Client class as well.