2
\$\begingroup\$

Description:

Design a book library which maintains the book inventory. The library should allow an user to favorite/un-favorite book. The library should allow an user to rent a book for a given period of time, no other user can rent the same book for overlapping period. There can be multiple copies of a book.

At present the code looks mess to me because all the logic is cramped into a single class, this is intentional and I would like to learn how to the decompose classes like these because sometimes we may need to work on some legacy code.

Code:

import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.Map.Entry;
import java.util.HashMap;
import java.util.Set;
import java.util.HashSet;
import java.util.UUID;
import java.util.Date;

class LibraryService {
  private final Map<String, Integer> bookQuantity;
  private final Map<String, Book> books;
  private final Map<User, BookRent> rentedBooks;
  private final Map<User, Set<Book>> favorites;

  LibraryService() {
    books = new HashMap<>();
    favorites = new HashMap<>();
    bookQuantity = new HashMap<>();
    rentedBooks = new HashMap<>();
  }

  void addBook(Book book, int quantity) {
    assert quantity > 0;

    String title = book.getTitle();
    books.put(title, book);
    bookQuantity.put(title, quantity);
  }

  void removeBook(Book book) throws Exception {
    // pre-condition
    for (Map.Entry<User, Set<Book>> pair : favorites.entrySet()) {
      removeFavorite(pair.getKey(), book);
    }
    String title = book.getTitle();
    books.remove(title);
    bookQuantity.remove(title);
  }

  void rent(User user, String title, Date start, Date end) throws Exception {
    assert start.before(end);

    Book found = findByTitle(title);
    if (found == null) {
      throw new Exception("Book not found");
    }
    decreaseQuantity(found);
  }

  void returnBook(User user, String title) throws Exception {
    Book found = findByTitle(title);
    if (found == null) {
      throw new Exception("Book not found");
    }
    increaseQuantity(found, 1);
  }

  Book findByTitle(String title) {
    return books.get(title);
  }

  public void favorite(User user, Book book) throws Exception {
    // the book should be in stock
    Book found = findByTitle(book.getTitle());
    if (found == null) {
      throw new Exception("Book not found");
    }
    if (favorites.get(user) == null) {
      favorites.put(user, new HashSet<Book>() {{
        add(book);
      }});
    } else {
      favorites.get(user).add(book);
    }
  }

  public Set<Book> getFavorites(User user) {
    return favorites.get(user);
  }

  public boolean removeFavorite(User user, Book book) {
    return favorites.get(user).remove(book);
  }

  private void increaseQuantity(Book book, int quantity) {
    String title = book.getTitle();
    Integer prevQuantity = bookQuantity.get(title);
    if (prevQuantity == null) {
      prevQuantity = 0;
    }
    bookQuantity.put(title, prevQuantity + quantity);
  }

  private void decreaseQuantity(Book book) throws Exception {
    String title = book.getTitle();
    Integer prevQuantity = bookQuantity.get(title);
    if (prevQuantity == null || prevQuantity == 0) {
      throw new Exception("Book out of stock");
    }
    bookQuantity.put(title, prevQuantity - 1);
  }
}

final class Book {
  private final UUID uid;
  private final String title;
  private final String author;

  Book(String title, String author) {
    this.title = title;
    this.author = author;
    uid = UUID.randomUUID();
  }

  String getTitle() {
    return title;
  }

  String getAuthor() {
    return author;
  }

  public String toString() {
    return "Book: " + this.getTitle() +
      " (" + this.getAuthor() + ")";
  }
}

class User {
  private final UUID uid;
  private final String name;

  User(String name) {
    this.name = name;
    uid = UUID.randomUUID();
  }
}

Questions:

  1. Where can I have custom exception classes?
  2. Where to catch exception vs throw them?
  3. When and where I can have assert statement?
  4. When can I pass book instance vs book title?

PS: Review should be done keeping in mind that the code is in production and used by thousand of users.

Note:

I am thinking to implement the feature to include date range while renting a book so, when renting a book the dates should not overlap. This feature is not in the scope but would be great if anyone can suggest me something.

\$\endgroup\$
  • 1
    \$\begingroup\$ Books from different authors may have the same title... \$\endgroup\$ – Sharon Ben Asher Oct 2 '17 at 7:07
  • \$\begingroup\$ "PS: Review should be done keeping in mind that the code is in production and used by thousand of users." Where's the persistent data storage? \$\endgroup\$ – Mast Oct 2 '17 at 17:49
  • \$\begingroup\$ @Mast that's implementation detail, teach me how to not think about it. \$\endgroup\$ – CodeYogi Oct 2 '17 at 18:21
1
\$\begingroup\$

taking the classic OO approach, Let's look at the data classes and the relations between them:

User:
properties: id, name
User has list of favorite Books. User can browse the list, and add/remove a Book from the list.
This relation is represented as Set<Book> favorites
User has list of rented BookCopyies. Each rental is valid for a time period.
User can browse the list, and add/remove a BookCopy from the list (rent/return Book)
This relation is represented as Set<BookCopy> rentals
(it is possible for a user to rent multiple copies of the same book. book copy is identified by copy-id)

User class is responsible for maintaining favorites and rentals.
operations:
boolean favorite(Book book) will return true/false if operation succeeded
boolean unFavorite(Book book) will return true/false if operation succeeded
boolean rent(Book book) will return true/false if operation succeeded
boolean return(Book book) will return true/false if operation succeeded

Book
properties: id, author, title
Book has one or more BookCopyies
This relation is represented as Set<BookCopy> inventory
optiojal: Book is favorite of multiple Users.
this will allow the service to show a User other Users with similar interests
(perhaps User can set his/her favorites as public or private)
This relation is represented as Set<User> favoredBy

Book class is responsible for maintaining its inventory of copies.
that includes adding and removing copies.
it should be able to answer if there is an available copy for rental for given period
BookCopy getAvailable(Date start, Date end) will return null if there is no available copy
the actual rental operation is done on a BookCopy!

BookCopy
properties: id, book-id
BookCopy may be rented to one User for a time period.
This relation is represented as User rentedBy and Date start, Date end

BookCopy class is responsible for maintaining its own rental status.
it should be able to answer if it is available for rental for given period boolean isAvailable(Date start, Date end)
it should be able to be rented for a given period
boolean rent(User user, Date start, Date end)
it should be able to be returned
boolean returned()
all operations return true/false if operation succeeded

LibraryService
LibraryService holds list of Books
LibraryService holds list of Users
LibraryService is able to identify Book by its id or author + title
This relation is represented as Map<UUID, Book> booksById and Map<AuthorTitle, Book> booksByAuthorTitle
AuthorTitle is helper class that holds author + title and is implements equals()
LibraryService is able to identify User by its id or name (putting aside for the moment that multiple users can have the same name)
This relation is represented as Map<UUID, User> usersById and Map<String, User> usersByName

LibraryService class is responsible for maintaining lists of Books and Users. that includes adding and removing items fron those lists (maintaining the maps)

so here is the flow of renting a book:

LibraryService: boolean rent(String userName, String author, String title, Date start, Date end)
identify User by userName return false if not found
identify Book by AuthorTitle return false if not found
call User.rent(book)
User: rent(Book book, Date start, Date end)
call book.getAvailable(start, end) and get BookCopy or null
call bookCopy.rent(this, start, end)
Book: getAvailable(Date start, Date end)
loop on inventory and for each item ask if (copy.isAvailable(start, end)) return first copy that answers true
BookCoopy: rent(User user, Date start, Date end)
set properties.

\$\endgroup\$
0
\$\begingroup\$

I've taken a run at your domain you have there and have tried to address most of your problems. It is in C# however. I hope that's OK. I wrote in Java a few years ago and I find them similar enough.

  • I did not attempt to implement the rental time span you alluded to.
  • I did not attempt a persistence layer, since the original code didn't have one.

I did try to address your domain concerns. I separated the functionality of each "Map" into a separate class and functions that use multiple maps in another class. I ended up with: BookIndex, InventoryIndex, FavoriteIndex, and Library. Seemed straight forward enough.

I also added custom exceptions to replace the generic exceptions. As well as some try/catch at the service level. And I move the assertions to be co-located with the logic they are protecting.

I also used a custom comparer for the User type so that I could use it as a key in a Map. You can't use reference types as a key in a dictionary without some form of custom comparison logic.

In regard to passing a Book instead of a title. I only found one instance on "RemoveFavorite" where a book was being passed when only a title was needed. In general, only ask for what is needed. Not the thing that has what is needed dangling off of it.

Anyway, I hope you find this break down useful. There were some scenarios in the original logic that were not accounted for that I tried to implement.

// NOTE: none of this gives any consideration to concurrency.  if you are indeed using this logic in a service
//       you should consider using thread safe Maps and locks.
// NOTE: your "service" layer should be very thin and only concerned with validating input into your domain
//       and logging exceptions on the way out.
public class LibraryService
{   
    private readonly Library library = new Library();

    public void AddBook(Book book, int quantity)
    {
        try
        {
            library.AddBook(book, quantity);
        }
        catch (Exception e)
        {
            // NOTE: log this to somewhere and do not suppress it.
            // NOTE: do this on every service operation if you want to know the details of what went wrong.
            throw  e;
        }
    }

    public void RemoveBook(Book book)
    {
        library.RemoveBook(book);       
    }

    public void RentBook(User user, String title, DateTime start, DateTime end)
    {
        library.Rent(user, title, start, end);
    }

    public void ReturnBook(User user, String title)
    {
        library.Return(user, title);
    }

    public Book FindByTitle(String title)
    {
        // NOTE: this will return a null result.  consider a Maybe<T> type construct.
        // NOTE: title isn't a very unique key for a book and may find more than
        //       one result.
        return library.Books.Find(title);
    }

    public void Favorite(User user, Book book)
    {
        library.Favorites.Add(user, book);
    }

    public List<Book> GetFavorites(User user)
    {
        return library.Favorites.Get(user);
    }

    // NOTE: a whole book isn't required here.  just a title.
    public void RemoveFavorite(User user, Book book)
    {       
        library.Favorites.Remove(user, book.Title);
    }
}

//---------------------------------------------------
// CUSTOM EXCEPTIONS
//---------------------------------------------------

public class TitleOutOfStockException : Exception
{
    public TitleOutOfStockException(string title) : base($"Book out of stock: {title}")
    {
    }
}

public class TitleNotFoundException : Exception
{
    public TitleNotFoundException(string title) : base($"Book not found: {title}")
    {
    }
}

//---------------------------------------------------
// DATA TYPES & COMPARERS
//---------------------------------------------------

public class User
{
    public readonly Guid Uid;
    public readonly String Name;

    public User(String name)
    {
        Name = name;
        Uid = Guid.NewGuid();
    }
}

// NOTE: C# allows custom comparers for it's collections.  i'm not sure what the java equivalent is.
public class UserComparer : IEqualityComparer<User>
{
    public static UserComparer Default = new UserComparer();

    public bool Equals(User a, User b)
    {
        return a.Uid == b.Uid;
    }

    public int GetHashCode(User u)
    {
        return u.Uid.GetHashCode();
    }
}

public class Book
{
    public readonly Guid Uid;
    public readonly String Title;
    public readonly String Author;

    public Book(String title, String author)
    {
        Title = title;
        Author = author;
        Uid = Guid.NewGuid();
    }

    public override String ToString()
    {
        return $"Book: {this.Title} ({this.Author})";
    }
}

//---------------------------------------------------
// DOMAIN LOGIC
//---------------------------------------------------

public class BookIndex
{
    // NOTE: i went case insensitive throughout the code
    private readonly Dictionary<String, Book> books = new Dictionary<String, Book>(StringComparer.CurrentCultureIgnoreCase);

    public void ValidateExistance(String title)
    {
        Book found;
        if (!books.TryGetValue(title, out found))
            throw new TitleNotFoundException(title);
    }

    public Book Find(String title)
    {
        Book found;
        return books.TryGetValue(title, out found)
            ? found
            : null;
    }

    public void Add(Book book)
    {
        Book found;

        if (!books.TryGetValue(book.Title, out found))
        {
            books[book.Title] = book;
        }
    }

    public void Remove(String title)
    {
        books.Remove(title);
    }
}

public class InventoryIndex
{
    private readonly Dictionary<String, Int32> inventory = new Dictionary<String, Int32>(StringComparer.CurrentCultureIgnoreCase);

    public void ValidateAvailablity(String title)
    {
        int found;

        if (!inventory.TryGetValue(title, out found) || found < 1)
            throw new TitleOutOfStockException(title);
    }

    public void Increase(String title, int quantity)
    {
        if (quantity < 1) throw new ArgumentException("Quantity must be greater than 0");

        int found;

        if (inventory.TryGetValue(title, out found))
            inventory[title] = found + quantity;
        else
            inventory[title] = quantity;
    }

    public void Decrease(String title, int quantity)
    {
        if (quantity < 1) throw new ArgumentException("Quantity must be greater than 0");

        int found;

        if (inventory.TryGetValue(title, out found))
            inventory[title] = (found - quantity) > 0
                ? found - quantity
                : 0;
        else
            inventory[title] = quantity;
    }

    public void Increment(String title)
    {
        Increase(title, 1);
    }

    public void Decrement(String title)
    {
        Decrease(title, 1);
    }

    public void Remove(String title)
    {
        inventory.Remove(title);
    }
}

public class FavoriteIndex
{
    // NOTE: in C# you can use reference types as keys if you supply a custom comparer. 
    private readonly Dictionary<User, List<Book>> favorites = new Dictionary<User, List<Book>>(UserComparer.Default);

    public void Add(User user, Book book)
    {
        List<Book> found;

        if (!favorites.TryGetValue(user, out found))
        {
            found = favorites[user] = new List<Book>();
        }

        if (found.FindIndex((b) => StringComparer.CurrentCultureIgnoreCase.Equals(b.Title, book.Title)) == -1)
        {
            found.Add(book);
        }
    }

    public List<Book> Get(User user)
    {
        List<Book> found;

        return favorites.TryGetValue(user, out found)
            ? found
            : new List<Book>();
    }

    public void Remove(User user, String title)
    {
        List<Book> found;

        if (favorites.TryGetValue(user, out found))
        {
            var index = found.FindIndex((b) => StringComparer.CurrentCultureIgnoreCase.Equals(b.Title, title));

            if (index > -1)
            {
                found.RemoveAt(index);
            }
        }
    }

    public void Remove(String title)
    {
        foreach (var user in favorites.Keys)
            Remove(user, title);
    }
}

public class Library
{
    public readonly BookIndex Books = new BookIndex();
    public readonly FavoriteIndex Favorites = new FavoriteIndex();
    public readonly InventoryIndex Inventory = new InventoryIndex();

    public void AddBook(Book book, int quantity)
    {
        Books.Add(book);
        Inventory.Increase(book.Title, quantity);
    }

    public  void RemoveBook(Book book) 
    {
        Favorites.Remove(book.Title);
        Inventory.Remove(book.Title);
        Books.Remove(book.Title);
    }

    public void Rent(User user, String title, DateTime start, DateTime end)
    {
        if (start >= end) throw new ArgumentException("Start must be before end.");

        Books.ValidateExistance(title);
        Inventory.ValidateAvailablity(title);
        Inventory.Decrement(title);     
    }

    public void Return(User user, String title)
    {
        Books.ValidateExistance(title);
        Inventory.Increment(title);     
    }
}
\$\endgroup\$
  • \$\begingroup\$ I see that all the request is delegated to library object, what is the use of libraryService then? \$\endgroup\$ – CodeYogi Oct 6 '17 at 6:36
  • 1
    \$\begingroup\$ @CodeYogi i usually interpret the term "service" to be some sort of technical endpoint. e.g. an http or wcf layer. if that's true, it's that classes job to vet incoming data, and log outgoing errors, and nothing else. IMO. Those classes are typically harder to test. Which is another good reason to keep your logic elsewhere. if it's not that kind of service, then there is no point to it in my version. \$\endgroup\$ – TBone Oct 6 '17 at 11:08
  • \$\begingroup\$ So, your service here is acting as a controller in web application? \$\endgroup\$ – CodeYogi Oct 6 '17 at 11:48
  • \$\begingroup\$ @CodeYogi That's what I assumed your service was doing. You mentioned production and thousands of users. It sounded like a service application to me. At minimum, your service (as written) is a Facade pattern. Accumulating inventory, user, and rental behavior into one bigger thing. Facades can be useful, if the domain is large enough. Otherwise, there just an annoying, ever-growing pass-through layer that has to be maintained. \$\endgroup\$ – TBone Oct 6 '17 at 11:56

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.