Problem
I want to optimize code in my simple app that I’m writing to learn C#. I feel it’s really badly optimized in terms of good coding practises. Could anyone take a look at it and give me a hint to make it look better?
public void mainMenu(BookList b)
{
bool repeat = false;
Console.WriteLine("Welcome to Library.");
Console.WriteLine("What do you want to do?");
Console.WriteLine("1. Add a book.");
Console.WriteLine("2. Delete book.");
Console.WriteLine("3. Borrow a book.");
Console.WriteLine("4. Return book.");
Console.WriteLine("5. See list of available books.");
Console.WriteLine("6. See list of all books.");
Console.WriteLine("7. Exit");
string userChoice = Console.ReadLine();
switch (userChoice)
{
case "1":
do
{
b.addBook();
Console.WriteLine("What do you want to do now?");
Console.WriteLine("Add another book? - Press 1.");
Console.WriteLine("Go back to main menu? - Press any other button.");
ConsoleKeyInfo keyPressed;
keyPressed = Console.ReadKey();
if (keyPressed.Key == ConsoleKey.D1 || keyPressed.Key == ConsoleKey.NumPad1)
{
repeat = true;
Console.Clear();
}
else
{
repeat = false;
}
}
while (repeat == true);
Console.Clear();
this.mainMenu(b);
break;
case "2":
b.deleteBook();
break;
case "3":
b.borrowBook();
break;
case "4":
b.returnBook();
break;
case "5":
b.displayBooks();
break;
}
}
I wanted to add repeating an action in addbook() function, but mainMenu is method of different class so I figured it out this way.
Solution
Starting with a small nitpick, that the rest of the application code already has — use full names for variables. Avoid the habit of short variable names like b. Feel free to use the full name of boardList instead. Remember that readability, clarity, and simplicity are key in programming.
Remember to always guard your inputs. We don’t know if bookList is null or not and we are attempting to use it by calling a method on it. This will cause a NullReferenceException. Simply check if it is null before using it.
The repeat variable is sticking out to me. It is declared at the top, but only used in one of the case statements. Using a break statement to control the do-while loop would be a cleaner approach.
case "1":
do
{
b.addBook();
Console.WriteLine("What do you want to do now?");
Console.WriteLine("Add another book? - Press 1.");
Console.WriteLine("Go back to main menu? - Press any other button.");
ConsoleKeyInfo keyPressed;
keyPressed = Console.ReadKey();
if (keyPressed.Key != ConsoleKey.D1 && keyPressed.Key != ConsoleKey.NumPad1)
{
Console.Clear();
break;
}
} while (true);
Console.Clear();
this.mainMenu(b);
break;
The next issue with the code is the line this.mainMenu(b);. This causes a recursive call that can fill the stack if called many many times (via the input “1” -> “any key but 1” -> repeat) or worse, cause some interesting bugs later on.
Try adding another console output after this.mainMenu(b);, hit the above path a couple of times, and try to figure out what happens.
A way to address this is to remove the line this.mainMenu(b);, make this mainMenu(...) function private and create a different public method that repeats the call to mainMenu(...).
Finally, we can add some helper functions with decent names and enums to help define what case "1": actually means. This helps to eliminate the magic, hard-coded, numbers to some degree (they are just a nature of the beast when dealing with these types of menus).
When we put this all together, we get a different approach. Remember that this is only based on the code I’ve seen, so take it as an example/another way to do the same thing, not an objectively better solution.
// Map user input (0, 1, 2...) to human-readable action names
// The actions should match up with the menuText string.
private enum MainMenuAction : int
{
invalid = 0,
repeat = 1,
addBook = 1,
deleteBook = 2,
// TODO etc.. fill the rest in
exit = 7,
}
// What we want our main menu to say.
// The numbers should match up with the MainMenuAction enum.
private static readonly string menuText =
"Welcome to Library.n" +
"What do you want to do?n" +
"1. Add a book.n" +
"2. Delete a book.n" +
"TODO etc... fill it out heren" +
"7. Exit.";
public void mainMenu(BookList bookList)
{
// Make sure bookList is not null, since we will be operating on it.
if (bookList == null)
{
// TODO Handle null bookList
return;
}
// Do our application-level looping here
do
{
// Display the main menu text
Console.WriteLine(menuText);
// Get the user input
int userChoice;
if (tryGetInput(out userChoice))
{
// If user wants to exit, then exit the main menu.
if (userChoice == (int)MainMenuAction.exit)
{
break;
}
// Handle the user input by casting the int to a human-readable name
handleMainMenuAction((MainMenuAction)userChoice, bookList);
}
else
{
// TODO Handle unexpected input (parsing to int failed, user error)
}
} while (true);
}
private void handleMainMenuAction(MainMenuAction action, BookList bookList)
{
// Make sure bookList is not null, since we will be operating on it.
if (bookList == null)
{
// TODO Handle null bookList
return;
}
// Ensure that the enum is defined
if (!Enum.IsDefined(typeof(MainMenuAction), action))
{
// TODO Handle undefined enum (user error)
return;
}
switch (action)
{
case MainMenuAction.addBook:
// Command-level looping
do
{
bookList.addBook();
} while (askUserForRepeat("Add another book"));
break;
case MainMenuAction.deleteBook:
bookList.deleteBook();
break;
// TODO Fill out the other cases
case MainMenuAction.invalid:
default:
// TODO Handle invalid action (internal error -- not yet implmented, probably)
break;
}
}
// Just a wrapped version of int.TryParse that grabs from the command line
private bool tryGetInput(out int input)
{
return int.TryParse(Console.ReadLine(), out input);
}
// Displays 'repeatText' as the first option, the second option is to go back to the main menu
// Basically a yes/no continue loop where 1 = continue and anything else = quit
private bool askUserForRepeat(string repeatText)
{
Console.WriteLine("1. " + repeatText);
Console.WriteLine("2. Back to Main Menu");
int userInput;
if (tryGetInput(out userInput))
{
if (userInput == (int)MainMenuAction.repeat)
{
return true;
}
else
{
// Go back to main menu -- user ended repeat.
return false;
}
}
else
{
// Go back to main menu -- invalid user input.
return false;
}
}
Your application is a nice candidate for the command pattern. With a few changes you can make it easily extendable and maintainable. Here’s how it goes:
First you need an interface for such a command. Each command needs at least a descritpiton and it must be able to do something:
interface ICommand
{
string Description { get; }
void Execute(BookList books);
}
Next we can start implementing the commands:
class AddBookCommand : ICommand
{
public string Description => "Add a book.";
public void Execute(BookList books) { }
}
class DeleteBookCommand : ICommand
{
public string Description => "Delete book.";
public void Execute(BookList books) { }
}
class ExitCommand : ICommand
{
public string Description => "Exit.";
public void Execute(BookList books) { Environment.Exit(0); }
}
Then in your Main you could create all the commands:
var commands = new ICommand[]
{
new AddBookCommand(),
new DeleteBookCommand(),
new ExitCommand()
};
a list of books:
var books = new BookList();
and the main loop:
while (true)
{
Console.WriteLine("Welcome to Library.");
Console.WriteLine("What do you want to do?");
// This loop creates a list of commands:
for (int i = 0; i < commands.Length; i++)
{
Console.WriteLine("{0}. {1}", i + 1, commands[i].Description);
}
// Read until the input is valid.
var userChoice = string.Empty;
var commandIndex = -1;
do
{
userChoice = Console.ReadLine();
}
while (!int.TryParse(userChoice, out commandIndex) || commandIndex > commands.Length);
// Execute the command.
commands[commandIndex - 1].Execute(books);
}
This is just a start. You still need to add console maintenance like .Clear some more error checking etc.
Indentation
Your indentation is off here (it may just be a copy/paste error, however it’s worth mentioning, remember consistency is key):
public void mainMenu(BookList b)
{
Naming
Public methods should follow the Pascal naming convention, so mainMenu should really be MainMenu, addBook=>AddBook etc.
InnerLoop
The inner loop under the case 1 is weird and the way it uses recursion to call the menu method again may cause problems with the stack if you add too many books in a single session.
Unhandled menu items
Your menu has 7 items in it, however you only handle items up to 5. This means 6 is actually the same as 7 in that it exits the menu. As there is no validation around the selected menu options, any other entry from the user (for example ‘help’) would also result in exiting the menu.
Consistently Clear
After you add a book, then the screen is cleared before adding another / before re-displaying the main menu. The main menu doesn’t contain a clear at the top though, meaning that the first time it is displayed it may or may not be on a clear console window (depending on what happened before mainMenu was called).
Separation of concerns
Generally speaking, you want to separate your user interactions from the core logic of your application. From your comment this doesn’t appear to be the case in your application “addBook() asks for name, author, release date etc”. This couples your logic to the user interface and makes it difficult to change in the future. If you want to change the user interface to a web page, or WPF you have to go in and change the behaviour of your BookList. Try to think about how you can separate out your application into different areas of responsibility (user interaction being one of them) so that it is easier to maintain going forward. A step towards doing this is breaking down your code into methods with meaningful names that describe what each method does.
It is also a good idea to think about how you might want to drive the code if you didn’t have a user interface (unit testing can be a good way to practice this). This can lead you to thinking of a BookList in a slightly different way, so rather than having void addBook(), you might have BookIdentifier AddBook(Book newBook). Then deleteBook() might become DeleteBook(BookIdentifier) etc. Where Book represents information like ‘Author, Release Date’ etc and BookIdentifier is some unique book identifer (for example its ISBN, or a unique catalog id etc).
Numbers for selection
One of the problems with using numbers for menus is that it can create a maintenance overhead. If you decided to add another menu item ‘See list of borrowed books’, then you probably wouldn’t want it to be displayed after exit so you would make it number 7, and have to update the text for ‘7. Exit’ and update the case statement that handles that selection. This can be error prone. @t3chb0t has provided a good solution to this. I’ve included another alternative, largely for contrast below.
First I create a simple class for holding details about a menu option:
public class MenuOption
{
public MenuOption(string itemText, Action itemHandler, bool isExitOption = false)
{
ItemText = itemText;
ItemHandler = itemHandler;
IsExitOption = isExitOption;
}
public string ItemText { get; }
public Action ItemHandler { get; }
public bool IsExitOption { get; }
}
This holds three pieces of information. The text that describes the menu option, an Action (something to do when the option is selected) and a flag to indicate if this action represents the exit menu selection (this defaults to false for no).
I then create a helper method to build a list of menu items. Notice that the action in most cases is simply the name of the method on bookList to invoke, with the exception of “Exit”:
static List<MenuOption> BuildMainMenu(BookList bookList)
{
return new List<MenuOption> { new MenuOption("Add a book.", bookList.addBook),
new MenuOption("Delete book.", bookList.deleteBook),
new MenuOption("Borrow a book.", bookList.borrowBook),
new MenuOption("Exit", ()=>Console.WriteLine("Goodbye"), true)
};
}
And another helper to display the list of items. This loops round the built list, printing each item, along with its selection number, so if the items change order in the list, their actions will still match their selection:
private static void DisplayMainMenu(List<MenuOption> options)
{
Console.Clear();
Console.WriteLine("Welcome to Library.");
Console.WriteLine("What do you want to do?");
int optionCount = 1;
foreach (var option in options)
{
Console.WriteLine($"{optionCount++}.{option.ItemText}");
}
}
One more helper to get a selection from the user:
private static MenuOption GetMenuSelection(List<MenuOption> options)
{
int selection;
do
{
string userChoice = Console.ReadLine();
if (int.TryParse(userChoice, out selection) &&
selection > 0 &&
selection <= options.Count)
{
return options[selection - 1];
}
Console.WriteLine("Sorry, Try again");
}
while (true);
}
Note that the above has some error checking to make sure a valid selection is picked by the user. The MainMenu function then simply becomes a loop that continues until the user selects the exit option:
public void MainMenu(BookList bookList)
{
bool exitMenu = false;
var menu = BuildMainMenu(bookList);
while (!exitMenu) {
DisplayMainMenu(menu);
var menuChoice = GetMenuSelection(menu);
menuChoice.ItemHandler?.Invoke();
exitMenu = menuChoice.IsExitOption;
}
}