3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Do addBook, deleteBook etc interact with the user as well? \$\endgroup\$ – forsvarir Sep 27 '16 at 17:24
  • \$\begingroup\$ Yes. Every single method asks for input from user. For example addBook() asks for name, author, release date etc. \$\endgroup\$ – Easy Sep 27 '16 at 17:27
2
\$\begingroup\$

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 here\n" +
    "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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your time and input. I'm total beginner and you shed new light on my approach to coding :). I surely will return with more code to codereview :P \$\endgroup\$ – Easy Sep 27 '16 at 18:56
  • \$\begingroup\$ No problem, welcome to the code review community! Remember to up-vote well-formed answers and to accept the answers that most benefit you. Most importantly, pay it forward by reviewing and voting on other posts in the community! Best of luck. \$\endgroup\$ – JD.B Sep 27 '16 at 19:03
  • 1
    \$\begingroup\$ One more question with recursive call on this.mainMenu(b) The problem is that it would stack this. keyword after many loops and would basicly make stacks of mainMenu methods called? Anyway, could you also give me your opinion on my approach of creating List? I basically create new instance of List inside BookList class class BookList { List<Book> books = new List<Book>(); \$\endgroup\$ – Easy Sep 27 '16 at 19:12
  • \$\begingroup\$ Think of it like this. When this.mainMenu(b); is called, that version of the code and variables is suspended (it is placed on the stack) and a new version is pushed on top of it. When we return from the new version, the new version is popped from the stack. And the first, suspended version, is resumed. Since there is no code to execute, it immediately returns and is also popped from the stack. At which point the execution returns to the caller (which is whoever called mainMenu. Hard to explain via text, so just ask if you need clarification. Recursion is a hard topic. \$\endgroup\$ – JD.B Sep 27 '16 at 19:22
  • \$\begingroup\$ That second part is tricky. As a general rule of thumb, think about "What if I want to represent my books with a linked list or hash set rather than a list?". The way you described works just fine, I personally use that approach often. The little bit of overhead of having to redefine Add(Book book) and such is a small price to pay for the readability and flexibility. Remember to always program towards the abstraction. \$\endgroup\$ – JD.B Sep 27 '16 at 19:31
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ It's still under construction and I'm adding more options all the time. I know it's unhandled yet, but I am covering it as I'm expanding my methods to check if everything works fine. \$\endgroup\$ – Easy Sep 27 '16 at 22:49
  • \$\begingroup\$ As to BookIdentifier you mentioned. Does it mean you recommend creating another Class that stores just BookIdentifiers as reference to each object? \$\endgroup\$ – Easy Sep 27 '16 at 23:13
  • \$\begingroup\$ @Easy BookIdentifier could be a class, or it could simply be an int or a string, it really depends on how it is used and how complex it is. Some people would prefer to write the methods in terms of Book, so it is also a bit down to personal choice and design decisions. \$\endgroup\$ – forsvarir Sep 28 '16 at 6:05
  • \$\begingroup\$ I can't find a way to refference outside variable to a class object, is there any way or am I thinking about it in a wrong way. For now I had UniqueID property for each book that i used in deleteBook() method \$\endgroup\$ – Easy Sep 28 '16 at 21:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.