6
\$\begingroup\$

I decided that in order to get better at programming, I should focus on building and improving simple projects. I have made a library program that contains collections of unique books of varying quantities. Although this runs as expected, I am not satisfied with the potential of expanding it to complexities. What are the ways I can improve the efficiency of this program's code, and allow greater potential of increasing the complexity of it? Thank you.

Book.java

/*
 * This class defines the uniqueness of each individual book
 * that is part of a separate collection
 * 
 */

public class Book {
    
    private boolean borrowed; //Each book is either borrowed or not
    
    public Book() {
        this.borrowed = false;
    }
    
    public void setToBorrowed() { //Method will fire once a book has been checked out
        this.borrowed = true;
    }
    
    public void setToReturned() { //Method will fire once book has been returned to the library catalog
        this.borrowed = false;
    }
    
    public boolean isBorrowed() { //Determines whether the book is borrowed or not
        return this.borrowed;
    }
    
}

BookCollection.java


import java.util.ArrayList;
import java.util.List;

/*
 * The purpose of this class is to define a collection of books for different titles
 */

public class BookCollection {

    private int quantity; //Number of copies for each book collection
    
    List<Book> books = new ArrayList<>(); //Collection of individual books
    
    public BookCollection(int quantity) { //Creates a collection of books with a defined # of copies
        this.quantity = quantity;
        for(int i = 0; i < quantity; i++) {
            books.add(new Book());
        }
    }

    public void addBook() { //Adds a new book object to the collection of books
        books.add(new Book());
    }
    
    public boolean borrowBook() { //Borrows a book from the collection
        for(Book b : books) {
            if(!b.isBorrowed()) {
                b.setToBorrowed();
                return true; //Book has been borrowed successfully 
            }
            else {
                continue;
            }
        }
        System.out.println("All books are borrowed, sorry");
        return false; //Book has failed to be borrowed
    }
    
    public boolean returnBook() { //Returns a book back to the catalog
        for(Book b : books) {
            if(b.isBorrowed()) {
                b.setToReturned();
                return true; //Book has been returned successfully
            }
            else {
                continue;
            }
        }
        System.out.println("Cannot return book at this time, sorry!");
        return false; //Book has failed to be returned
    }
    
    public void printBooks() {
        for(Book b : books) {
            System.out.println("Borrowed? " + b.isBorrowed());
        }
    }
    
    public int getQuantity() { //Returns the # of copies the collection has, borrowed or not
        return this.quantity;
    }
    
}

Library.java

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Scanner;

/*
 * This class is the engine of the program
 */

public class Library {
    
    static Map<BookCollection, String> bookCatalog =
            new HashMap<BookCollection, String>(); //Entire collection of books
    
    public static void main(String[] args) {
        
        Scanner console = new Scanner(System.in);
        
        //Test BookCollection objects:
        BookCollection book1 = new BookCollection(5);
        BookCollection book2 = new BookCollection(3);
        BookCollection book3 = new BookCollection(2);
        
        bookCatalog.put(book1, "Cracking the Coding Interview");
        bookCatalog.put(book2, "Crime and Punishment");
        bookCatalog.put(book3, "Catch-22");
    
        //Client borrows 2 copies of Catch-22
        borrowBook("Catch-22");
        borrowBook("Catch-22");
    
        
        printCatalog();
        System.out.println();
        
        //Client returns the copies
        returnBook("Catch-22");
        returnBook("Catch-22");
        
        
        //Test if catalog has been updated correctly
        printCatalog();
        System.out.println();
        
    }
    
    /*
     * printCatalog()
     * 
     * Return type: void
     * 
     * This method prints out each collection of books 
     */
    
    public static void printCatalog() { 
        
        for(Map.Entry<BookCollection, String> entry : bookCatalog.entrySet()) {
            
            BookCollection bc = (BookCollection) entry.getKey();
            
            System.out.println("Title: " + entry.getValue());
            bc.printBooks();
            System.out.println();
        }
    }
    
    /*
     * I am sure there is a way to simplify 'borrowBook()' and 'returnBook()' by use of a new method
     * that does what both of these already do (searching for a book title match)
     */
    
    /*
     * borrowBook(String bookTitle)
     * 
     * Return type: void
     * 
     * This method borrows a book from a collection
     */
    
    public static void borrowBook(String bookTitle) { //Borrows a book from the entire library catalog
        
        for(Map.Entry<BookCollection, String> entry : bookCatalog.entrySet()) {
            if(entry.getValue().equals(bookTitle)) {
                BookCollection bc = (BookCollection) entry.getKey();
                if(bc.borrowBook())
                    System.out.println("You have successfully borrowed " + bookTitle);
                else
                    System.out.println("All copies of " + bookTitle + " have been checked out already, sorry! :(");
                
                return;
            }
            else {
                continue;
            }
        }
    
        System.out.println(bookTitle + " doesn't exist - sorry!");
        
    }
    
    /*
     * returnBook(String bookTitle)
     * 
     * Return type: void
     * 
     * This method returns a book back to the collection of books
     * 
     */

    public static void returnBook(String bookTitle) {
        for(Map.Entry<BookCollection, String> entry : bookCatalog.entrySet()) {
            
            if(entry.getValue().equals(bookTitle)) {
                BookCollection bc = (BookCollection) entry.getKey();
                if(bc.returnBook())
                    System.out.println("You have successfully returned " + bookTitle);
                else
                    System.out.println(bookTitle + " cannot be returned at this time");
                
                return;
                
            }
            else {
                continue;
            }
        }
        
        System.out.println(bookTitle + " doesn't exist - sorry!");
        
    }
    
}
\$\endgroup\$
  • 2
    \$\begingroup\$ At first glance, you need to change most of your Library class code from static to non-static. Creating one instance of Library is more extensible than a static class. Also, your bookCatalog map belongs in a model class. The model / view / controller pattern is powerful when creating a Java application. en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller \$\endgroup\$ – Gilbert Le Blanc Aug 4 at 1:50
  • \$\begingroup\$ @GilbertLeBlanc I appreciate your comment and will study the MVC design pattern. I was concerned with my map's implementation and its lack of flow with the rest of the program. \$\endgroup\$ – calghir Aug 4 at 5:52
6
\$\begingroup\$

As suggested by Gilbert Le Blanc there are some useful structural patterns that can be applied in your case. But one thing at a time.

In your Library class there is this comment:

I am sure there is a way to simplify 'borrowBook()' and 'returnBook()' by use of new method that does what both of these already do (searching for a book title match)

You are right, there are some improvements that could be made on that part. The first one I see is the usage of your map; One advantage of a map is to quickly/efficiently retrieve an item by key. In your case the key is not the book, but the title.

private static static Map<String, BookCollection> booksByTitle = new HashMap<>();

// ...

public static void borrowBook(String bookTitle) {
    if ( !booksByTitle.containsKey(bookTitle) ) {
        System.out.println(bookTitle + " doesn't exist - sorry!");
        return;
    }
    
    BookCollection collection = booksByTitle.get(bookTitle);
    if(collection.borrowBook())
        System.out.println("You have successfully borrowed " + bookTitle);
    else
        System.out.println("All copies of " + bookTitle + " have been checked out already, sorry! :(");
}

Once done, you see that the two methods borrowBook and returnBook are similar, only the operation and message is different. You can then refactor your code to extract that code :

private static void doInCollection(String title, Consumer<BookCollection> operation) {
    if (!booksByTitle.containsKey(title)) {
        System.out.println(title + " doesn't exist - sorry!");
        return;
    }
    operation.accept(booksByTitle.get(title));
}

(Added with edit 1:) That you use with :

public void borrowBook(String bookTitle) {
    doInCollection(bookTitle, collection -> {
        if (collection.borrowBook())
            System.out.println("You have successfully borrowed " + bookTitle);
        else
            System.out.println("All copies of " + bookTitle + " have been checked out already, sorry! :(");
    });
}

At that time, you should convert the methods from your Library to instance methods and took that opportunity to encapsulate the booksByTitle map.

public static void main(String[] args) {
    Library library = new Library();
    library.add(5, "Cracking the Coding Interview");
    library.add(3, "Crime and Punishment");
    library.add(2, "Catch-22");

    library.borrowBook("Catch-22");
    library.borrowBook("Catch-22");

    library.printCatalog();
    System.out.println();

    //Client returns the copies
    library.returnBook("Catch-22");
    library.returnBook("Catch-22");

    //Test if catalog has been updated correctly
    library.printCatalog();
    System.out.println();
}     

At this time, we can wonder what is the role of your Book class ? It is just a wrapper around a boolean. You are also printing them as a boolean while, it would be easier to print the number of copie available and borrowed.

So you can remove the Book class and manage two counters in your
BookCollection.

class BookCollection {
    private int quantity;
    private int available;
    
    // ...
    public boolean borrowBook() { //Borrows a book from the collection
        if ( available>0 ) {
            available -= 1;
            return true;
        } else {
            System.out.println("All books are borrowed, sorry");
            return false;
        }
    }
    
    // ...
}

Now, if you want to apply the MVC pattern, your BookCollection seems to be a good candidate for the model while the Library looks like a controller once all call to System.out are moved to a dedicated (view) class.

Another improvement would be to use Exceptions and query methods instead of boolean to manage exceptional cases.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the very helpful suggestions. I have not seen the 'Consumer' interface in Java until now, so I am trying to figure out its role in your reply. in the 'doInCollection' method, what does operation.accept() entail? \$\endgroup\$ – calghir Aug 4 at 19:39
  • 1
    \$\begingroup\$ Let's say that Consumer is a function that receive one parameter (BookCollection in our case) and return nothing. The lambdas syntax (>=Java 8) allow you to create one with a simple syntax: parameter -> { body }. I will edit my answer to add this example. \$\endgroup\$ – gervais.b Aug 5 at 6:00
  • \$\begingroup\$ I just want to make sure I understand your example of functional programming because this concept is new to me. With the code doCollection(bookTitle, collection -> {//Code}); are we telling the program to pass in the result of an accepted BookCollection value into a block of code that will later check if a book can be borrowed or not? Thank you again for your response. \$\endgroup\$ – calghir Aug 6 at 0:48
  • 1
    \$\begingroup\$ Yes, you have understood it well. \$\endgroup\$ – gervais.b Aug 6 at 10:02

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.