1
\$\begingroup\$

I'm building an application to manage book rentals. Can someone check if I used encapsulation correctly?

LibApp.java

package com.company;

import com.company.model.Book;
import com.company.model.BookDirectory;
import com.company.model.RentDirectory;
import com.company.users.Admin;
import com.company.users.Customer;

public class LibApp {
    public static void main(String[] args) {    
        BookDirectory b = new BookDirectory();
        RentDirectory r = new RentDirectory();
        Book book = new Book("Title3","Author3");
        b.getBookDirectory().add(book);
        Admin admin = new Admin("Admin1");
        admin.addBook("Title1", "Author1");
        admin.addBook("Title2", "Author2");
        admin.addCustomer("Name1");
        Customer customer = new Customer("Name2");
        Customer c = new Customer("Name3");
        customer.rentBook("Title1");
        customer.returnRentedBook("Title1");
        customer.rentBook("Title2");
        c.rentBook("Title2");    
    }
}

User.java

package com.company.users;

public abstract class User {
    protected int id;
    protected String name;
    protected int privilege;
    protected static int userIdCounter = 1;
}

Customer.java

package com.company.users;

import com.company.model.Book;
import com.company.model.BookCustomerRentList;
import com.company.service.BookRenter;

public class Customer extends User {
    private BookCustomerRentList rentedBooksByCustomer;
    BookRenter bookRenter = new BookRenter();
    Book book = new Book("");

    public Customer(String name){
        this.id = userIdCounter;
        this.name = name;
        this.privilege = 0;
        this.rentedBooksByCustomer = new BookCustomerRentList();
        userIdCounter++;
    }

    public void rentBook(String title){

        if(bookRenter.doRentBookSucceed(title)){
            book.setTitle(title);
            rentedBooksByCustomer.getBooksRentedByCustomer().add(book);
        }

    }

    public void returnRentedBook(String title){
        book.setTitle(title);
        if (rentedBooksByCustomer.getBooksRentedByCustomer().contains(book)){
            rentedBooksByCustomer.getBooksRentedByCustomer().remove(book);
        }
    }

}

Admin.java

package com.company.users;

import com.company.model.Book;
import com.company.service.BookCreator;
import com.company.service.CustomerCreator;

public class Admin extends User {
    Book book = new Book("", "");
    CustomerCreator customerCreator = new CustomerCreator();
    BookCreator bookCreator = new BookCreator();

    public Admin(String name) {
        this.name = name;
        this.id = userIdCounter;
        this.privilege = 1;
        userIdCounter++;
    }

    public void addCustomer(String name) {
        customerCreator.createCustomer(name);
    }

    public void addBook(String title, String author) {
        bookCreator.createBook(title, author);
    }
}

Book.java

package com.company.model;

public class Book {
    private int id;
    private String title;
    private String author;
    protected static int bookIdCounter = 1;

    public Book(String title, String author){
        this.id = bookIdCounter;
        this.title = title;
        this.author = author;
        bookIdCounter++;
    }

    public Book(String title) {
        this.title = title;
    }

    public int getId() {
        return id;
    }

    public String getTitle() {
        return title;
    }

    public String getAuthor() {
        return author;
    }

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

BookCustomerRentList.java

package com.company.model;

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

public class BookCustomerRentList {
    private List<Book> booksRentedByCustomer = new ArrayList<Book>();

    public List<Book> getBooksRentedByCustomer() {
        return booksRentedByCustomer;
    }
}

BookDirectory.java

package com.company.model;

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

public class BookDirectory {
    private static List<Book> bookDirectoryList = new ArrayList<Book>();
    private RentDirectory rentedBooksList = new RentDirectory();
    private Book book = new Book("");

    public boolean isBookAvailableToRent(String title){
        boolean checkIsBookAvailableToRent = false;
        book.setTitle(title);

        if (rentedBooksList.isBookToRentInRentDirectory(title)) {
            checkIsBookAvailableToRent = false;
        }
        else if (isBookToRentInBookDirectory(title)){
            checkIsBookAvailableToRent = true;
        }
        return checkIsBookAvailableToRent;
    }

    boolean isBookToRentInBookDirectory(String title) {
        Book book = new Book(title);
        boolean checkIsBookInBookDirectory = false;

        for (Book bookInBookDirectory : bookDirectoryList) {
            if (bookInBookDirectory.getTitle().equals(book.getTitle())) {
                checkIsBookInBookDirectory = true;
            }
            else {
                checkIsBookInBookDirectory = false;
            }
            return checkIsBookInBookDirectory;
        }
        return checkIsBookInBookDirectory;
    }

    public List<Book> getBookDirectory() {
        return bookDirectoryList;
    }
}

RentDirectory.java

package com.company.model;

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

public class RentDirectory {
    private static List<Book> rentedBooksDirectory = new ArrayList<Book>();

    public List<Book> getRentedBooks() {
        return rentedBooksDirectory;
    }

    public boolean isBookToRentInRentDirectory(String title) {
        Book book = new Book(title);
        boolean checkIsBookInRentDirectory = false;

        for (Book bookInRentDirectory : rentedBooksDirectory) {
            if (bookInRentDirectory.getTitle().equals(book.getTitle())) {
                checkIsBookInRentDirectory = true;
            }
            else {
                checkIsBookInRentDirectory = false;
            }
            return checkIsBookInRentDirectory;
        }
        return checkIsBookInRentDirectory;
    }
}

UserDirectory.java

package com.company.model;

import com.company.users.User;

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

public class UserDirectory {
    private static List<User> userDirectory = new ArrayList<User>();

    public List<User> getUserDirectory() {
        return userDirectory;
    }
}

BookCreator.java

package com.company.service;

import com.company.model.Book;
import com.company.model.BookDirectory;

public class BookCreator {

    BookDirectory bookDirectory = new BookDirectory();

    public void createBook(String title, String author){
        Book book = new Book(title, author);
        bookDirectory.getBookDirectory().add(book);
    }
}

BookRenter.java

package com.company.service;

import com.company.model.Book;
import com.company.model.BookDirectory;
import com.company.model.RentDirectory;

public class BookRenter {
    Book book = new Book("");
    BookDirectory bookDirectory = new BookDirectory();
    RentDirectory rentedBooksDirectory = new RentDirectory();

    public boolean doRentBookSucceed(String title){

        boolean checkDoRentBookSucceed;
        book.setTitle(title);

        if (bookDirectory.isBookAvailableToRent(title)){
            rentedBooksDirectory.getRentedBooks().add(book);
            checkDoRentBookSucceed = true;
            System.out.println("You have rented a \"" + book.getTitle() + "\" book.");
        }
        else {
            checkDoRentBookSucceed = false;
            System.out.println("The book is not available.");
        }
        return checkDoRentBookSucceed;
    }
}

CustomerCreator.java

package com.company.service;

import com.company.model.UserDirectory;
import com.company.users.Customer;

public class CustomerCreator {
    UserDirectory userDirectory = new UserDirectory();

    public void createCustomer(String name){
        Customer customer = new Customer(name);
        userDirectory.getUserDirectory().add(customer);
    }
}
\$\endgroup\$
2
\$\begingroup\$

Encapsulation

Did I use encapsulation correctly?

No, and it's easy to see just by looking at how you use the classes:

BookDirectory b = new BookDirectory();
RentDirectory r = new RentDirectory();
Book book = new Book("Title3","Author3");
b.getBookDirectory().add(book);

Here, the BookDirectory exposes its internal state by returning List<Book>. A malicious user could do this:

b.getBookDirectory().clear();

Poof, the books in directory are gone. With good encapsulation, the code would look like this:

b.addBook(book);

This way the internal structure is not exposed. Equally important that this would also make BookDirectory more natural to use.


Admin admin = new Admin("Admin1");
admin.addBook("Title1", "Author1");
admin.addBook("Title2", "Author2");
admin.addCustomer("Name1");

The admin adds books and a customer. But adds to where, exactly? What about the BookDirectory and RentDirectory instances we saw earlier?

Looking at the implementation, Admin has a BookCreator instance, which in turn has a BookDirectory instance, and these will be used instead of the BookDirectory created earlier.

The Admin should not be the owner of the BookDirectory and the UserDirectory. It would make more sense if there was one BookDirectory and one UserDirectory in the application, independent from the Admin. That would be a more realistic model, because in reality admin's don't own the book collection or the user registry, it's the library that does.

For example, it will make more sense t have a Library class, that has a BookDirectory and UserDirectory. Admin users could receive a BookCreator and a CustomerCreator instance from the Library.


Customer customer = new Customer("Name2");
Customer c = new Customer("Name3");
customer.rentBook("Title1");
customer.returnRentedBook("Title1");
customer.rentBook("Title2");
c.rentBook("Title2");

Similar to the previous issue, customers are created without any oversight by the library or the admin. The CustomerCreator object of the admin is completely bypassed. Unauthorized customers can rent books from the library, which makes the concept of the admin and its CustomerCreator pointless in the model.


The above are serious issues and they were easily visible just by looking it how you use the classes. Looking deeper into the implementation reveals many more related similar issues, just by thinking about how the different components use each other, and asking if that usage is natural, and if the overall model is natural.

So how to do better? I think you need to rethink the overall model, the main classes in it, and the relationships between them. Write down the fields that each class should have, and draw arrows to express dependencies. For example, this could be part of such diagram:

 ---------
| Library |
 ---------       ----------------
| books   |---> | BookRepository |
 ---------       ----------------
                | books: Map     |
                 ----------------
                | add(Book)      |
                | ...            |
                 ----------------

The fewer arrows, the better. For example, probably no other class should have access to the BookRepository instance of the Library. That could lead to inconsistent state. All operations on the BookRepository should go through the Library

Keep in mind the two main goals of encapsulation:

  • Grouping of data and operations on that data
  • Information hiding: not exposing implementation details that users should not have to know about, and should be kept private

The first part is easy, and you got that right, more or less.

The second part is a bit harder. Trying to reduce the number of arrows will you guide in that direction. Question every arrow in the diagram if it really needs to be there. Does object X should really know about object Y, or should that be encapsulated by another object?

Bug

This loop doesn't loop:

for (Book bookInRentDirectory : rentedBooksDirectory) {
    if (bookInRentDirectory.getTitle().equals(book.getTitle())) {
        checkIsBookInRentDirectory = true;
    }
    else {
        checkIsBookInRentDirectory = false;
    }
    return checkIsBookInRentDirectory;
}

It checks only the first book. You probably meant to write like this:

for (Book bookInRentDirectory : rentedBooksDirectory) {
    if (bookInRentDirectory.getTitle().equals(book.getTitle())) {
        return true;
    }
}
return false;

Notice that I removed the unnecessary (and tediously named) flag variable checkIsBookInRentDirectory.

Flag variables

Avoid flag variables when possible. For example instead of this:

public boolean doRentBookSucceed(String title){

    boolean checkDoRentBookSucceed;
    book.setTitle(title);

    if (bookDirectory.isBookAvailableToRent(title)){
        rentedBooksDirectory.getRentedBooks().add(book);
        checkDoRentBookSucceed = true;
        System.out.println("You have rented a \"" + book.getTitle() + "\" book.");
    }
    else {
        checkDoRentBookSucceed = false;
        System.out.println("The book is not available.");
    }
    return checkDoRentBookSucceed;
}

This would be better:

public boolean doRentBookSucceed(String title){

    book.setTitle(title);

    if (bookDirectory.isBookAvailableToRent(title)){
        rentedBooksDirectory.getRentedBooks().add(book);
        System.out.println("You have rented a \"" + book.getTitle() + "\" book.");
        return true;
    }
    else {
        System.out.println("The book is not available.");
        return false;
    }
}

Naming

The names in the implementation are extremely tedious. Take for example the doRentBookSucceed of BookRenter. The function returns a boolean, so the notion of success or failure is already implied, without having the word "Succeed" in its name. More importantly, what's the main purpose of the BookRenter? In your code, it is to rent books. So the function could be called just rent.

\$\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.