Return to Revisions

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).

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 that may work better. 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,
    // etc..
    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 here\n" +
    "7. Exit.";

public void mainMenu(BookList bookList)
{
    // 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)
{
    // 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;

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