1
\$\begingroup\$

Over the past 2 days I've developed a simple library application, but I'm not sure about relations between classes I designed and where I can use composition,add interface or design pattern. I would like to get your feedback on the design.

package com.library.main;

import com.library.models.BookDirectory;
import com.library.models.Rent;
import com.library.service.Alert;
import com.library.service.FileGenerator;
import com.library.service.UserRegistration;
import com.library.users.Customer;
import com.library.users.User;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

public class RunApplication {

    public static void main(String[] args) {
        BookDirectory bookDirectory = new BookDirectory();
        HashSet<User> userSet = new HashSet<User>();
        HashSet<Customer> customerSet = new HashSet<Customer>();
        FileGenerator.setBookDirectory(bookDirectory);
        UserRegistration.setUsers(userSet);
        UserRegistration.setCustomers(customerSet);
        Alert.setCustomers(customerSet);
    }
}
package com.library.models;

public class Author {

    private static int nextAuthorId = 0;

    private String name;
    private int id;

    public Author(String name) {
        this.name = name;
        id = nextAuthorId;
        nextAuthorId++;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }
}
package com.library.models;

public class Book {

    private BookTitle bookTitle;
    private int id;

    public Book(BookTitle bookTitle, int id) {
        this.bookTitle = bookTitle;
        this.id = id;
    }

    public BookTitle getBookTitle() {
        return bookTitle;
    }

    public String getTitle() {
        return getBookTitle().getTitle();
    }

    public void setTitle(String title) {
        this.getBookTitle().setTitle(title);
    }

    public Author getAuthor() {
        return getBookTitle().getAuthor();
    }

    public void setAuthor(Author author) {
        this.getBookTitle().setAuthor(author);
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

}
package com.library.models;

import com.library.users.Customer;

import java.io.FileWriter;
import java.io.IOException;
import java.time.LocalTime;
import java.time.Period;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;

public class BookDirectory {
    private ArrayList<Book> books;
    private HashSet<Author> authors;
    private HashSet<BookTitle> bookTitles;

    public void addBook(BookTitle bookTitle, int id) {
        Book newBook = new Book(bookTitle, id);
        books.add(newBook);
    }

    public boolean hasBook(Book book) {
        return books.contains(book);
    }

    public void deleteBook(Book book) {
        books.remove(book);
    }

    public void deleteAllBooks() {
        books.clear();
    }

    public int getBookCount() {
        return books.size();
    }

    public void addAuthor(String name) {
        authors.add(new Author(name));
    }

    public void clearBookDirectory() {
        books.clear();
        authors.clear();
    }

    public void addBookTitle(String title, Author author, int id) {
        bookTitles.add(new BookTitle(title, author, id));
    }

    public int getBookTitlesCount() {
        return bookTitles.size();
    }

    public String toString() {
        return books.toString();
    }

    public List<Book> getAllBooks() {
        return books;
    }

    public HashSet<BookTitle> getAllBookTitles() {
        return bookTitles;
    }

}
package com.library.models;

public class BookTitle {
    private String title;
    private Author author;
    private int id;

    public BookTitle(String title, Author author, int id) {
        this.title = title;
        this.author = author;
        this.id = id;
    }

    public String getTitle() {
        return title;
    }

    public void setTitle(String title) {
        this.title = title;
    }

    public Author getAuthor() {
        return author;
    }

    public void setAuthor(Author author) {
        this.author = author;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }
}
package com.library.models;

import com.library.users.Customer;

import java.time.LocalDate;
import java.time.Period;

public class Rent {
    private Customer customer;
    private Book book;
    LocalDate dateOfRent;
    Period period;

    public Rent(Customer customer, Book book, Period period) {
        this.customer = customer;
        this.book = book;
        this.period = period;
        dateOfRent = LocalDate.now();
    }

    public Customer getCustomer() {
        return customer;
    }

    public void setCustomer(Customer customer) {
        this.customer = customer;
    }

    public Book getBook() {
        return book;
    }

    public void setBook(Book book) {
        this.book = book;
    }

    public LocalDate getDateOfRent() {
        return dateOfRent;
    }

    public void setDateOfRent(LocalDate dateOfRent) {
        this.dateOfRent = dateOfRent;
    }

    public Period getPeriod() {
        return period;
    }

    public void setPeriod(Period period) {
        this.period = period;
    }
}
package com.library.models;

public enum UserPrivilige {
    USER, ADMIN, SUPERADMIN
}

package com.library.service;

import com.library.models.Rent;
import com.library.users.Customer;
import com.library.users.User;

import java.time.LocalDate;
import java.time.Period;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.Set;

public class Alert {

    private static HashSet<Customer> customers;

    public ArrayList<Rent> getOverdueRents() {
        return getSoonOverdueRents(Period.ZERO);
    }

    public static HashSet<Customer> getCustomers() {
        return customers;
    }

    public static void setCustomers(HashSet<Customer> customers) {
        Alert.customers = customers;
    }

    public ArrayList<Rent> getSoonOverdueRents(Period period) {
        ArrayList<Rent> result = new ArrayList<Rent>();
        for (Customer customer : customers) {
            for (Rent rent : customer.getRentedBooks()) {
                if (rent.getDateOfRent().plus(rent.getPeriod()).isAfter(LocalDate.now().minus(period))) {
                    result.add(rent);
                }
            }
        }
        return result;
    }

    public ArrayList<Rent> getAllRents() {
        ArrayList<Rent> result = new ArrayList<Rent>();
        for (Customer customer : customers) {
            for (Rent rent : customer.getRentedBooks()) {
                    result.add(rent);
            }
        }
        return result;
    }
}
package com.library.service;

import com.library.models.UserPrivilige;
import com.library.users.User;

import java.util.HashSet;
import java.util.List;
import java.util.Set;

public class Authentification {
    private Set<User> userSet;
    public Authentification(Set<User> userSet){
        this.userSet = userSet;
    }

    public boolean isLoginValid(String name, String password) {
        for (User user : userSet) {
            if (user.getName().equals(name) && user.getPassword().equals(password)) {
                return true;
            }
        }
        return false;
    }

    public boolean isAdmin(User user) {
        return user.getPrivilige().compareTo(UserPrivilige.ADMIN) >= 0;
    }
}
package com.library.service;

import com.library.models.Book;
import com.library.models.BookDirectory;
import com.library.models.BookTitle;
import com.library.models.Rent;
import com.library.users.Customer;
import com.library.users.User;

import java.io.FileWriter;
import java.io.IOException;
import java.time.LocalTime;
import java.util.HashSet;

public class FileGenerator {
    private static BookDirectory bookDirectory = null;

    public static BookDirectory getBookDirectory() {
        return bookDirectory;
    }

    public static void setBookDirectory(BookDirectory bookDirectory) {
        FileGenerator.bookDirectory = bookDirectory;
    }

    public static void createDirectoryBookListFile() {
        try {
            FileWriter fileWriter = new FileWriter("book_list_" + LocalTime.now().toString() + ".txt");
            fileWriter.write("Title\tAuthor\tQuantity\n");
            for (BookTitle bookTitle : bookDirectory.getAllBookTitles()) {
                Integer bookQuantity = 0;
                for (Book book : bookDirectory.getAllBooks()) {
                    if (book.getBookTitle().equals(bookTitle))
                        bookQuantity++;
                }
                fileWriter.write(bookTitle.getTitle() + "\t" + bookTitle.getAuthor() +
                        "\t" + bookQuantity.toString() + "\n");
            }
            fileWriter.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public void createCustomerBookListFile(Customer customer) {
        try {
            FileWriter fileWriter = new FileWriter("book_list_" + customer.getName() + "_" +
                    LocalTime.now().toString() + ".txt");
            fileWriter.write("Book\tDate of Rent\tRent Period\n");
            for (Rent rent : customer.getRentedBooks()) {
                fileWriter.write(rent.getBook().getTitle() + "\t" + rent.getDateOfRent() +
                        "\t" + rent.getPeriod() + "\n");
            }
            fileWriter.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}
package com.library.service;

import com.library.models.Book;
import com.library.models.BookDirectory;

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

public class Search {
    private BookDirectory dir;

    public Search(BookDirectory dir) {
        this.dir = dir;
    }

    public List<Book> search(String phrase) {
        List<Book> results = new ArrayList<Book>();
        for (Book b : dir.getAllBooks()) {
            if (b.getAuthor().getName().equals(phrase) || b.getTitle().equals(phrase))
                results.add(b);
        }
        return results;
    }
}
package com.library.service;

import com.library.users.Customer;
import com.library.users.User;

import java.util.HashSet;
import java.util.List;
import java.util.Set;

public class UserRegistration {
    private String name;
    private String email;
    private String password;

    private static HashSet<User> users;
    private static HashSet<Customer> customers;

    public static HashSet<User> getUsers() {
        return users;
    }

    public static void setUsers(HashSet<User> users) {
        UserRegistration.users = users;
    }

    public static HashSet<Customer> getCustomers() {
        return customers;
    }

    public static void setCustomers(HashSet<Customer> customers) {
        UserRegistration.customers = customers;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getEmail() {
        return email;
    }

    public void setEmail(String email) {
        this.email = email;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public boolean isRegistrationValid() {
        int nameLength = name.length();
        int passwordLength = password.length();
        boolean passwordHasDigit = password.matches("\\d+");
        boolean isEmailValid = email.matches("^[a-zA-Z0-9._+-]+@[a-zA-Z._-]+.[a-zA-Z0-9]]$");
        return nameLength > 3 && passwordLength > 6 && passwordHasDigit && isEmailValid;
    }

    public boolean registerUser() {
        if (isRegistrationValid()) {
            boolean isNameFree = true;
            for (User user : users) {
                if (user.getName().equals(name)) {
                    isNameFree = false;
                    break;
                }
            }
            if (isNameFree) {
                users.add(new User(name, email, password));
                return true;
            }
        }
        return false;
    }

    public boolean registerCustomer() {
        if (!registerUser())
            return false;
        customers.add(new Customer(name, email, password));
        return true;
    }

}
package com.library.users;

import com.library.models.Book;
import com.library.models.UserPrivilige;

import java.util.ArrayList;

public class User {
    protected String name;
    protected String email;
    protected String password;
    private int id;
    private UserPrivilige privilige = UserPrivilige.USER;
    private static int nextUserId = 0;

    public User(String name, String email, String password) {
        this.name = name;
        this.email = email;
        this.password = password;
        this.id = nextUserId;
        nextUserId++;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getEmail() {
        return email;
    }

    public void setEmail(String email) {
        this.email = email;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public UserPrivilige getPrivilige() {
        return privilige;
    }

    public void setPrivilige(UserPrivilige privilige) {
        this.privilige = privilige;
    }
}
package com.library.users;

import com.library.models.Book;
import com.library.models.BookDirectory;
import com.library.models.Rent;

import java.io.FileWriter;
import java.io.IOException;
import java.time.LocalTime;
import java.time.Period;
import java.util.ArrayList;

public class Customer extends User {

    private ArrayList<Rent> rentedBooks;
    private static BookDirectory bookDirectory;

    public Customer(String name, String email, String password) {
        super(name, email, password);
        rentedBooks = new ArrayList<Rent>();
    }

    public static BookDirectory getBookDirectory() {
        return bookDirectory;
    }

    public static void setBookDirectory(BookDirectory bookDirectory) {
        Customer.bookDirectory = bookDirectory;
    }

    public ArrayList<Rent> getRentedBooks() {
        return rentedBooks;
    }

    public boolean rentBook(Book book, Period period) {
        if (bookDirectory.hasBook(book)) {
            getRentedBooks().add(new Rent(this, book, period));
            return true;
        }
        return false;
    }

    public void returnBook(Rent rent) {
        rentedBooks.remove(rent);
    }
}
package com.library.users;

import com.library.models.Author;
import com.library.models.Book;

public class Admin extends User {
    public Admin(String name, String email, String password) {
        super(name, email, password);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to StackExchange Code Review! Please review How do I ask a good Question? Specifically, it is best to explain what the code does. This is especially true in the title. \$\endgroup\$ – Stephen Rauch Apr 22 '17 at 20:31
  • \$\begingroup\$ Does this code actually do anything? It looks like you create three objects and set various static (class) fields to them. Then the program stops. Does this program actually take input or produce output somewhere? \$\endgroup\$ – mdfst13 Apr 23 '17 at 4:11
  • \$\begingroup\$ @mdfst13 not yet, i'm working on this. But I would like to know is my design approach going in a good direction \$\endgroup\$ – AJJack Apr 23 '17 at 7:04
1
\$\begingroup\$

Let me first point out that you should almost always choose the Interface over a concrete implementation. The concrete implementation should be used in 1 place only. That is where an instance is actally created. You did this correct in a few places like:

public List<Book> search(String phrase) {
    List<Book> results = new ArrayList<Book>();

You create an ArrayList and store it in a List, return it as a List and handle it like a List. This is good. But you should do this for all your List's, Set's and possibly other things.

Examples where you did it wrong:
private ArrayList<Rent> rentedBooks; -> private List<Rent> rentedBooks
private static HashSet<User> users; -> Set<User>
private static HashSet<Customer> customers; -> Set<Customer>
...
to name a few. (also includes the getters/setters).

In cases like this for (Customer customer : customers) { it makes sense to use Customer instead of User because you're specifically talking about the subclass. Other types of User's don't interest us in that loop. So that's OK.


Immutable is better than mutable. For some classes it makes no sense to ever change one of the fields. Take a BookTitle for example. I've never known a book title suddenly change. So change those fields to final and remove the setters:

public class BookTitle {
    private final String title;
    private final Author author;
    private final int id;

    public BookTitle(String title, Author author, int id) {
        this.title = title;
        this.author = author;
        this.id = id;
    }

    public String getTitle() {
        return title;
    }

    public Author getAuthor() {
        return author;
    }

    public int getId() {
        return id;
    }
}

The same can be done for Book, Author, Rent, UserRegistration, ...

It does have some implications though. Like if you make a Rent immutable, each time you want to "update" a rent (like extending the return date by a week) you'll have to create a new rent to replace the old one. Alternatively you could keep the renting period variable, or provide a special extendByAWeek() method to do just that.

Sometimes it's more logical that variables change. Like the period of a Rent or the email and adres of a user (people tend to move from time to time, or change their email adres). Some purists would still make the objects immutable, always creating a new instance with the updated value and replacing all instances. But I think that's too much of a hassle. I would start with immutable objects and whenever it makes more sense to have a value change (like when a user moves) provide the setter or other update method.


Another useful principle is YAGNI. Which stands for "You Aren't Going to Need It". Since your program doesn't do anything useful yet, it's hard to say which of these data structures you are going to need, and which you are not. That's why the initial responses to your question here on cr.se are: what is it for?

As long as you don't know what you need, you shouldn't implement it yet. Are you sure you will need that Admin class? What if you decide later on that the program will only be used by admins, and all you need is their login name (and password to make sure it's them). Then that means you didn't need the separation into Admin/Customer/User in the first place. You would just have Users (of the program) and Customers, which are 2 completely different things, each with their own information.

Just for your information: an Admin in your current program doesn't have the UserPrivilige.ADMIN. So what do those UserPriviliges do for you?



As for your choice of classes. For me it's not logical that a user is responsible of keeping track of the rented books. Your current program doesn't do much yet so it's hard to say which is the best place to keep track of the rentals. So depending on what you plan to do next with your program might conflict with what I'm going to propose here. So don't take this as a "you must do it this way". Instead look at how and why I choose certain things, and let it inspire you to improve your own program.


You have split up specific books with a general booktitle. It's not unusual for a library to have multiple of the same booktitle, and you don't care which of those you rent. So it makes sense to look up a booktitle and rent a specific copy of the book.

An alternative would be to just have Books (your current BookTiltes) and keep track of how many of those you have or who has ranted a copy of the book. I slightly prefer this alternative because you don't care about the individual copies.


The biggest change I'm thinking off is who keeps track of which book is rented?

You currently have a Customer that keeps track of the books that it has. You have a Rent class which maps a Book to a Customer and a period for when the book should be returned. There's a BookDirectory that could have just as well been a List<Book since it doesn't add any extra value at the moment. And some other classes that feel slightly out of place.

What you don't have is a Library class that contains the list of all books that are owned by the library. So currently your main method keeps track of those.

My idea is to first create that Library class. It has a Map that keeps track of the books that are for rent and how many of those are available. (Alternatively if you want to stick with your Book+BookTitle, this would be a Map<BookTitle, Set<Book>> that maps a title to the concrete available copies.)

Besides the available books this class also keeps a list of all the open Rent's (List). That way we have a central place to look up which books should be returned soon (instead of awkwardly looping over all the users in the system to find these), and we can also use this to find which users have a copy of a book if there's none available. Could be useful to tell a Customer: come back tomorow, John Smith will have brought back his copy by then.

The Search class (which has a rather bad name) becomes obsolete if we add it's public List<Book> search(String phrase) { to our new Library class. Alternatively we could rename the Search class to something more meaningful like BookFinder and have the Library class use this to search for a book in it's list. That way the library doesn't get a new responsibility (which, according to SOLID design principles is better). (Or even better, have the class responsible for handling the user's search request first get the list of books in the library, and then use the new BookFinder class to search for the matching books. So Library doesn't even need to know there is such a thing as "searching for a book".

Now the Customer class no longer needs to keep track of the books that this customer currently has. If you return a book, the librarian usually scans it to find out which book you're returning (with correct ID in the system etc...). This should now be handled in the Library class with a method like returnBook(Customer customer, Book book) that will delete the Rent with that combination of user and book and increases the book count in it's map of available books. Alternatively you could also keep this list in Customer. And when returning a book, the logic is that you take it out of customer and add it back in the library. This makes it easier to find out "what books does this customer currently have". (So again, the choice here depends on what your program is supposed to do).


as stated in the YAGNI part. You probably don't need the Admin or User classes. Start out with just Customer with the required fields. And later on see if you want to combine both Customer and User into a general Person. But then also look at where you want to replace the subclass (Customer) to the super class (Person). I doubt this will happen in many places since inside the program you'll always handle Customer's that rent books.

This also affects your UserRegistration class. Actually this class itself is implemented really weird. Here's what I would expect this class to look like:

public class CustomerRegistration {
    private Set<Customer> customers;

    public UserRegistration() {
        //load up the current existing users.
        //from a file or database
        //or hardcoded here for testing until actual loading is implemented
    }

    public Set<Customer> getAllCustomers(){
        return customers;
    }

    public boolean registerCustomer(String name, String email) {
        if(!isValidCustomerInfo(name, email){
            return false;
        }
        Customer customer = new Customer(name, email);
        customers.add(customer);
        //save customer to file/database  
        return true;
    }

    private boolean isValidCustomerInfo(String name, String email) {
        //check customer info here
    }
}

Most noticable is that it doesn't have fields for the name/email/password. The exact implementation might change (like I removed the password) depending on what you want to do with a Customer.

My initial thought is to not let them use the system themselves. Just have a librarian input handle the returning/checking out of the books.


Hope my explanation helps you decide what your program should and shouldn't do. It's hard to give advice about a program without having it's intention you know ;)

\$\endgroup\$

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.