8
\$\begingroup\$

I've made a very simple book list console app in C#. You can add the name, author and the page number of the book. You can also edit the infos or remove the book from the list if you want to. The code is working but I think it's very messy and don't know how to make it better.

using System;
using System.Collections.Generic;
using System.Linq;

namespace booklist
{
    class Book
    {
        public string Title { get; set; }
        public string Author { get; set; }
        public int Page { get; set; }

        public Book (string title_in, string author_in, int page_in)
        {
            this.Title = title_in;
            this.Author = author_in;
            this.Page = page_in;
        }

        public override string ToString()
        {
            return $"\nTitle: {Title}\nAuthor: {Author}\nPage: {Page}\n\n";
        }
    }

    class Display
    {
        public static void ShowBookList(List<Book> list)
        {
            Console.Clear();
            foreach(Book book in list)
            {
                Console.WriteLine($" {list.IndexOf(book) + 1}. book");
                Console.WriteLine("---------");
                Console.WriteLine(book);
            }
        }

        public static void Options(List<Book> list)
        {            
            do
            {
                if (BookList.empty)
                {
                    Console.Clear();
                    Console.WriteLine("\nThe list is now empty.\n");
                }
                Console.WriteLine("Write \"+\" if you want to add a book to the list.\n");                
                Console.WriteLine("Write \"-\" if you want to remove a book from the list.\n");
                Console.WriteLine("Write \"*\" if you want to edit a book info in the list.\n");
                Console.Write("Write \"q\" if you want to close the application. --> ");
                BookList.option = Console.ReadLine();
            } while (BookList.option != "+" && BookList.option != "q" && BookList.option != "-" && BookList.option != "*");

            switch(BookList.option)
            {
                case "+":
                    Display.AskForHowManyBooks();
                    BookList.AddBook(list, ref BookList.howMany, BookList.bookNumber, BookList.bookTitle, BookList.bookAuthor, BookList.bookPage);
                    BookList.empty = false;
                    break;

                case "q":
                    Console.WriteLine("\nOkay then, good bye.");
                    Environment.Exit(0);
                    break;

                case "-":
                    Console.Clear();
                    Console.Write("Which book do you want to remove from the list? [Write the number of the book] --> ");
                    do
                    {
                        BookList.number_ok = int.TryParse(Console.ReadLine(), out BookList.bookNumber);
                    } while (!BookList.number_ok);
                    if ((BookList.bookNumber <= list.Count) && (list.ElementAtOrDefault(BookList.bookNumber - 1) != null))
                    {
                        Console.WriteLine(BookList.books[BookList.bookNumber - 1]);
                        do
                        {
                            Console.Write("Are you sure you want to remove this book from the list? [Y/N] --> ");
                            BookList.confirmRemove = Console.ReadLine().ToUpper();
                        } while (BookList.confirmRemove != "Y" && BookList.confirmRemove != "N");
                        switch (BookList.confirmRemove)
                        {
                            case "Y":
                                BookList.RemoveBook(list, BookList.bookNumber - 1);
                                break;
                            case "N":
                                Display.ShowBookList(list);
                                Display.Options(list);
                                break;
                        }                                                
                    }
                    if((list != null) && (!list.Any()))
                    {
                        BookList.empty = true;                        
                        Display.Options(list);
                    }
                    break;

                case "*":
                    Console.Clear();
                    Console.Write("Select the book [Write the number of the book] --> ");
                    do
                    {
                        BookList.number_ok = int.TryParse(Console.ReadLine(), out BookList.bookNumber);
                    } while (!BookList.number_ok);
                    if ((BookList.bookNumber <= list.Count) && (list.ElementAtOrDefault(BookList.bookNumber - 1) != null))
                    {
                        Console.Write(BookList.books[BookList.bookNumber - 1]);
                        Book bookUpdated = BookList.BookInfo(BookList.bookNumber - 1, BookList.bookTitle, BookList.bookAuthor, BookList.bookPage);
                        BookList.books[BookList.bookNumber - 1] = bookUpdated;
                    }
                    break;
            }            
        }

        public static void AskForHowManyBooks()
        {
            do
            {
                Console.Clear();
                Console.Write("How many books do you want to add to the list? --> ");
                BookList.size_ok = BookList.HowManyBooks(Console.ReadLine(), ref BookList.howMany, BookList.MAX_SIZE);
            } while (!BookList.size_ok);
        }

        public static void AskForBookTitle(int bookNumber_in)
        {
            
            Console.Write($"\n{bookNumber_in + 1}. book - What is the title? --> ");
        }

        public static void AskForBookAuthor(int bookNumber_in)
        {
            
            Console.Write($"{bookNumber_in + 1}. book - Who wrote the book? --> ");
        }

        public static void AskForBookPage(int bookNumber_in)
        {
            
            Console.Write($"{bookNumber_in + 1}. book - How many pages does the book have? --> ");
        }
    }

    static class BookList
    {
        public const int MAX_SIZE = 100;
        public static string option;
        public static string confirmRemove;
        public static int bookNumber;
        public static string bookTitle;
        public static string bookAuthor;
        public static int bookPage;
        public static bool page_ok;
        public static bool size_ok;
        public static bool number_ok;
        public static int howMany;
        public static bool empty = BookList.IsListEmpty(books);
        public static List<Book> books = new List<Book>();

        public static bool IsListEmpty(List<Book> list)
        {
            if ((list != null) && (!list.Any()))
            {
                return false;
            }

            return true;
        }
        public static bool HowManyBooks(string size_in, ref int howMany_in, int max)
        {
            return int.TryParse(size_in, out howMany_in) && (howMany_in >= 0 && howMany_in <= max);
        }

        public static string GetBookTitle(string bookTitle_in)
        {
            do
            {
                bookTitle_in = Console.ReadLine();
            } while (string.IsNullOrEmpty(bookTitle_in));

            return bookTitle_in;
        }

        public static string GetBookAuthor(string bookAuthor_in)
        {
            do
            {
                bookAuthor_in = Console.ReadLine();
            } while (string.IsNullOrEmpty(bookAuthor_in));

            return bookAuthor_in;
        }

        public static int GetBookPage(int bookPage_in, bool? page_ok_in)
        {
            do
            {
                page_ok_in = int.TryParse(Console.ReadLine(), out bookPage_in);
            } while (page_ok_in == false);

            return bookPage_in;
        }

        public static Book BookInfo(int bookNumber_in, string bookTitle_in, string bookAuthor_in, int bookPage_in)
        {
            Book book_in;
            bool? page_ok_in = null;

            Display.AskForBookTitle(bookNumber_in);
            bookTitle_in = BookList.GetBookTitle(bookTitle_in);

            Display.AskForBookAuthor(bookNumber_in);
            bookAuthor_in = BookList.GetBookAuthor(bookAuthor_in);

            Display.AskForBookPage(bookNumber_in);
            bookPage_in = BookList.GetBookPage(bookPage_in, page_ok_in);

            book_in = new Book(bookTitle_in, bookAuthor_in, bookPage_in);

            return book_in;
        }

        public static void AddBook(List<Book> list, ref int howMany_in, int bookNumber_in, string bookTitle_in, string bookAuthor_in, int bookPage_in)
        {
            for (bookNumber_in = 0; bookNumber_in < howMany_in; ++bookNumber_in)
            {
                Book book = BookList.BookInfo(bookNumber_in, bookTitle_in, bookAuthor_in, bookPage_in);
                list.Add(book);
            }
        }

        public static void RemoveBook(List<Book> list, int bookNumber_in)
        {
            list.RemoveAt(bookNumber_in);          
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            while (BookList.empty)
            {               
                Display.Options(BookList.books);
            }
            
            while (!BookList.empty)
            {
                Display.ShowBookList(BookList.books);
                Display.Options(BookList.books);                
            }               
        }
    }
}
``` 
\$\endgroup\$
5
  • \$\begingroup\$ No permanent storage? Your program looks quite odd. What is the goal of the program? What prompted you to write it? Is it an exercise? \$\endgroup\$
    – Mast
    Jul 31, 2020 at 14:18
  • \$\begingroup\$ @Mast It's not an exercise. Actually I want to make a book list program with GUI to keep track of the books I've read but before that I wanted to do something with lists in C#. \$\endgroup\$ Jul 31, 2020 at 14:31
  • \$\begingroup\$ Won't the program forget your books the moment you exit? \$\endgroup\$
    – Mast
    Jul 31, 2020 at 15:46
  • \$\begingroup\$ Yes but I don't know how to store the data and retrieve it from a file. \$\endgroup\$ Jul 31, 2020 at 16:02
  • 1
    \$\begingroup\$ @EkremGulcan check this out docs.microsoft.com/en-us/dotnet/csharp/programming-guide/… \$\endgroup\$
    – Jessica
    Jul 31, 2020 at 19:58

1 Answer 1

11
\$\begingroup\$

Split classes into multiple files.

For the same reason that libraries allow users to rent individual books, programmers should split classes into individual files. It allows for them to be more easily digested and modified in the future.


Allowing for valid user input

Regarding this check BookList.option != "q" in Display.Options(), this only allows for lowercase input. Instead try using the following: BookList.option.Equals("q", StringComparison.OrdinalIgnoreCase);.

You will also need to update the switch case accordingly:

case "q":
case "Q":
    Console.WriteLine("\nOkay then, good bye.");
    //other code here

Return values

Instead of setting values in public variables when finding the result from a method, you should return it as a type instead.

For example, in the following method, you set BookList.size_ok, instead you should return this as a type from the method.

If you need to set a public variable, then you should set it from the result of the method.

public static bool AskForHowManyBooks()
{
    var sizeOk = false;
    do
    {
        Console.Clear();
        Console.Write("How many books do you want to add to the list? --> ");
        sizeOk = BookList.HowManyBooks(Console.ReadLine(), ref BookList.howMany, BookList.MAX_SIZE);
    } while (sizeOk);

    return sizeOk;
}

Parameter and variable naming

Maybe work on improving your naming conventions. Names of functions and classes seem to be ok, but lots of your variables, fields and parameters aren't great. Names should be clear and concise, and should avoid bad practices such as hungarian notation.

An example of a bad parameter name in your code: List<Book> list in the AddBook() function.

For more information, checkout Microsoft's guide on the matter.

Another example of names that don't follow C# convention:

public Book (string title_in, string author_in, int page_in)
{
    this.Title = title_in;
    this.Author = author_in;
    this.Page = page_in;
}

According to convention, the constructor arguments should be titled like so: titleIn.

EDIT: @Heslacher made a good point in the comments, you could even drop the in suffix. So the parameter would be called title.


Incorrect field placing?

This sounds (or smells) like a code smell to me, but you may have a valid justification that I'm unaware of?

In booklist, you are storing the following fields:

public static int bookNumber;
public static string bookTitle;

Surely these should be placed in book? Why would you have information about a specific book in a class relating to a list of books?


Use properties instead of fields

Try moving the fields mentioned in the previous section to properties so they can be properly accessed outside the class.

It doesn't make sense for them to be static fields.


Use var

This is more of a preference, but the .NET world is leaning towards it being a convention.

When setting new varaibles (for example in this foreach loop):

foreach(Book book in list)

It may be worth it to use var instead of explicitly stating the type.

There are many benefits to this, for more information checkout this article.


Many methods can be made private.

If you aren't accessing a method outside of the class, think about making it private.

About 30% of your methods could be made private, for example:

public static string GetBookTitle(string bookTitle_in)
{
    do
    {
        bookTitle_in = Console.ReadLine();
    } while (string.IsNullOrEmpty(bookTitle_in));

    return bookTitle_in;
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ "According to convention, the constructor arguments should be titled like so: titleIn." I would go one step further and skip the In completely. Otherwise +1 \$\endgroup\$
    – Heslacher
    Jul 31, 2020 at 15:58
  • 2
    \$\begingroup\$ Good point, will update, thanks. \$\endgroup\$
    – Jessica
    Jul 31, 2020 at 16:01

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.