Book program with arrayList

Question

I have this Book program that contains 2 classes: Book and Library. I have the Book class done, but need some help on the Library class. Please help me check my code. I can provide the Book class if you need it.

Instruction here:

Fields: A single private ArrayList of Book field is all that is necessary. This will hold all the Book objects in the library.

Methods: public Library(ArrayList other)

Throws a NullPointerException if other is null. Otherwise, the Library’s Book ArrayList should take on all the books in other.

public Library( ) Creates an empty ArrayList of books.

public boolean add(Book book)

First checks for null or empty Strings and calls the appropriate exception. Otherwise it adds the Book argument to the end of the library ArrayList. Notice it returns a boolean (whether or not the add operation succeeded) See the add method of ArrayList:

public ArrayList findTitles(String title)

Generates an ArrayList of all books which have titles that match (exactly) with the passed argument and returns this list to the calling program. The String compareTo method is useful here.

public void sort( )

Sorts the library’s book ArrayList in ascending order according to the title field (sort these titles just as they are, i.e. don’t worry about articles such as The or A, etc.). As illustrated in the textbook, p. 666 (Don’t let this number concern you :) ), you will use the Collections sort.

public String toString( )

returns a properly formatted String representation of all the books in the library (Title followed by authors).

    import java.util.ArrayList;

import java.util.Collections;

public class Library {
    private ArrayList<Book> allBook = new ArrayList<Book>();

    public Library(ArrayList<Book> other) {
        if (other == null) {
            throw new NullPointerException("null pointer");
        } else
            this.allBook = other;
    }

    public Library() {
        this.allBook = new ArrayList<Book>();
    }

    public boolean add(Book book) {
        if (book != null && !book.equals("")) {
            throw new IllegalArgumentException("Can't be empty");
        }
        allBook.add(book);
        return true;
    }

    public ArrayList<Book> findTitles(String title) {
        for(Book b: allBook) {
            if(title.compareTo(b.getTitle())== 0) {
                return allBook;
            }
        }
        return null;
    }

    public void sort() {
        Collections.sort(allBook);
    }

    public String toString() {
        return Library.this.toString();
    }
}

    public class Book implements Comparable<Book> {
    private String bookTitle;
    private ArrayList<String> bookAuthor;

    public Book(String title, ArrayList<String> authors) {
        if(title == null && authors == null) {
            throw new IllegalArgumentException("Can't be null");
        }
        if(title.isEmpty() && authors.isEmpty()) {
            throw new IllegalArgumentException("Can't be empty");
        }
        bookTitle = title;
        bookAuthor = authors;
    }

    public String getTitle() {
        return bookTitle;
    }
    public ArrayList<String> getAuthors( ) {
        return bookAuthor;
    }

    public String toString( ) {
        return bookTitle + bookAuthor;
    }
    public int compareTo(Book other){
        return bookTitle.compareTo(other.bookTitle);
    }
    public boolean equals(Object o) {
         if(!(o instanceof Book)) {
             return false;
        }
         Book b = (Book) o;
         return b.bookTitle.equals(bookTitle)
                 && b.bookAuthor.equals(bookAuthor);
    }
}

Answer 1

Your default constructor Library() initializes the allBook member variable twice, with the same value in each case. Probably not what you had in mind. The immediate fix is to remove the (re) assignment in the default constructor.

Now you have two different constructors that do the right thing two different ways. To save yourself maintenance headaches, you'd normally prefer to have one code path that does "everything", so it would be good to combine the two. There are two different ways of doing that, depending on the requirements....

Consider this test.

ArrayList source = new ArrayList();
source.add(book1);
source.add(book2);
source.add(book3);

Library library = new Library(source);
library.add(book4);

At this point, the library should contain four books. But how many books should the source array contain?

If the answer is four, the the library is supposed to be modifying the array it was passed. In that case, the easy answer to the constructor problem is to have the default constructor call the other, like so:

Library () {
    this(new ArrayList<Book>);
}

On the other hand, if the source array should still contain three books, then the Library should contain a copy of the ArrayList, rather than holding onto the original. If that's your requirement, then the good answer is to go the other way around - initialize the allBook member where you declare it, but use copy semantics when you are passed the ArrayList

ArrayList<Book> allBook = new ArrayList<Book> ();

Library () {}

Library (ArrayList<Book> books) {
    allBook.addAll(books);
}

Your code in this version of the puzzle is very confused about Books, titles, and Strings. Your requirements said

public ArrayList findTitles(String title)

but the signature in your class is

public ArrayList<Book> findTitles(Book title)

The code in a number of places suggests that, once upon a time, you thought all books were just Strings, and then you changed your mind. Your compiler should be telling you that you aren't being consistent.

The requirements for findTitles says that you should be returning an ArrayList of books with matching titles. That means you probably need to be creating a new ArrayList, and then add()ing the matching Books to it. The code you've written returns all of the books, but takes extra time to compare at the books first. You probably don't want to call Book.compareTo (although you can implement the solution correctly that way), but instead Book.getTitle().equals(otherBook.getTitle())

There are better answers than for loops for visiting all of the elements in an ArrayList. See Iterable. Since you've written Book.toString() already, you can start Library.toString() by building a big String out of all of the Book.toString()s.

Answer 2

I have never written a line of java code, but I'll give it a shot:

public Library(ArrayList<Book> other) {
    if (other == null) {
        throw new NullPointerException("null pointer");
    } else
        allBook = other;
}

The else here is redundant after the guard clause, this method could be written like this:

public Library(ArrayList<Book> other) {
    if (other == null) {
        throw new NullPointerException("null pointer");
    }
    allBook = other;
}

---

Library.toString() (spoiler):

Simply iterate allBook and call the toString method on each book.

The Book class' toString method could do a little more than just concatenating bookTitle+bookAuthor. Maybe something like "Title: " + bookTitle + " | Author: " + bookAuthor would look much better.

---

In Book.equals(), I think the method should actually throw an exception if o isn't a book, but again I'm not very used to duck-typed code so maybe not... but then if an object has a bookTitle and a bookAuthor property then why shouldn't it be treated as if it were a Book? It looks like a duck, quacks like a duck, walks like a duck, ...it's a duck! (or maybe I'm mixing up java with javascript here) -- point is, if Book.equals() handles the case where o isn't a Book, then I don't see why Book.compareTo() shouldn't do the same... or vice-versa!

Answer 3

Just a quick note (as far as I see nobody has mentioned yet):

ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

Answer 4

I agree with everything said by @VoidOfUnreason and @RetailCoder, and won't bother repeating them. I'll just note that there are many careless bugs.

In Library.add(), your if-condition is wrong. Also, the problem specifies that it should return a boolean indicating whether the operation succeeded. That's open to interpretation: what constitutes a failure? Running out of memory? Not likely; the customary action in that case would be to helplessly let the OutOfMemoryException propagate. What if you try to add the same book to the library twice? That's a reasonable failure mode. What if you add a second copy of a book that is already in the library? That should succeed, I would think.

In Library.findTitles(), your for-loop is wrong. A common idiom in languages with zero-based arrays is…

for (int i = 0; i < n; i++) { ... }

In any case, modern Java code would typically say instead…

for (Book b : allBook) { ... }

The specification says that .findTitles() should return a list of matching books — so where's your new list?

A hint for Library.toString(): you want to build a long string, so use a StringBuilder object. For each book in the library, you'll want to appends its string representation to the result.

In the Book() constructor, your validation probably doesn't behave the way you intended.

In Book.toString(), just slapping bookTitle and bookAuthor together is unlikely to yield a satisfactory result.

Since you overrode Book.equals(), you should also implement Book.hashCode() to be consistent. Otherwise, you would not be able to store Books in a HashMap or Hashtable.

I would rename your instance variables allBook and bookAuthor using their plurals. The code will read more smoothly, and logic mistakes will be less likely.

---

You really ought to have some test cases. Usually, that would be accomplished using JUnit, but for a beginner, making a public static void main(String[] args) would work fine. Implementing the tests would be a good exercise for you, and would help to uncover many bugs.

Answer 5

1) I know it is not your fault, but I strongly disagree with the specification

Methods: public Library(ArrayList other)

Throws a NullPointerException if other is null. Otherwise, the Library’s Book ArrayList should take on all the books in other.

I do not like the fact of killing objects in the constructor. The job of a constructor is: to initialize the object in a sane state - not killing it for user's faults. There are two ways to prevent this:

1) Use a factory or a builder method, which refuse to initialize the Library with null

2) Gracefully ignore the attempt to initialize with null and initialize with an empty ArrayList

Besides: Throwing a NullPointerException in the constructor leads to ugly code, since you have to wrap the construction in an unnecessary try-catch-block. Don't do this - please!

2) Naming You could improve your naming a little bit: * allBooks should be renamed to just books, since the ArrayList contains all books. allBooks is a tautology.

• add could be renamed to addBook - of course you could object, that this is a tautology too; since every IDE shows you, which type is expected. But not everyone uses an IDE. From this I would prefer addBook

3) Actual Code

First checks for null or empty Strings and calls the appropriate exception. Otherwise it adds the Book argument to the end of the library ArrayList. Notice it returns a boolean (whether or not the add operation succeeded) See the add method of ArrayList:

Again: These are very poor specifications! Why on earth do you have to return a boolean to signal, that there was no error, when the previous sentence clearly states, you have to throw an Exception if anything goes wrong? And more: Who cares for a boolean return-value? The only thing, which would make any sense, would be returning some kind of id in a persistence-context or for heavens sake the total number of books, but not a boolean.

I am unhappy with that implementation. Of course, your Book has a toString()-Method and you are checking if the book is literaly a blank book. But doing it this way:

public boolean add(Book book) {
    if (book != null && !book.equals("")) {
        throw new IllegalArgumentException("Can't be empty");
    }
    allBook.add(book);
    return true;
}

It reads wrong: »Check, whether the book is an empty string!« That doesn't make any sense at all. And honestly: At first, I didn't get it, unless I saw, the toString().

A better way of expressing this would be, adding a method to Book:

public boolean isBlank(){
    return bookAuthor.isEmpty() && bookTitle.isEmpty();
}

And the check becomes more readable and makes sense

public boolean addBook(Book book) {
    if (book == null || book.isBlank()){
        throw new IllegalArgumentException("Can't be empty");
    }
    allBook.add(book);
    return true;
}

»If the book is null or the book is blank throw an IllegalArgumentException« Btw. Why did you you use the negative from: !a && !b instead of just a || b?

Generates an ArrayList of all books which have titles that match (exactly) with the passed argument and returns this list to the calling program. The String compareTo method is useful here.

Here the specification is clear, but your implementation is wrong:

public ArrayList<Book> findTitles(String title) {
    for(Book b: allBook) {
        if(title.compareTo(b.getTitle())== 0) {
            return allBook;
        }
    }
    return null;
}

You iterate over all books in the library, and if one book matches, you return the whole library. That's not, what you intended. And more Do not return null! Please, never ever! That doesn't make any sense and leads to excessive use of Guard Clauses against null. It is a clear antipattern. In Java8 there is a nicer solution if you want to hide null in an optional.

Say, your teacher asks you to look for a specific book on the shelf and you do simply nothing - what does that mean to your teacher? A) you are deaf or b) you are dumb or c) you are neither, but there is no book. If someone asks you for a list of books, when there is none, simply return an empty list, and say politely: »Sorry, the list is empty, because there was no such book«.

public ArrayList<Book> findTitles(String title) {
    ArrayList<Book> result=new ArrayList<>();
    for(Book b: allBook) {
        if(title.compareTo(b.getTitle())== 0) {
            result.add(b);
        }
    }
    return result;
}

This would be the polite way to answer the question.

Sorts the library’s book ArrayList in ascending order according to the title field (sort these titles just as they are, i.e. don’t worry about articles such as The or A, etc.). As illustrated in the textbook, p. 666 (Don’t let this number concern you :) ), you will use the Collections sort.

Sorry, the specification makes me cry.

How does this make sense in any way? Imagine someone going to a library, with a wizards hat on and a big staff: »Sort you, I, as your master, command you!«.

public void sort() {
    Collections.sort(allBook);
}

Of course, the implementation is according to the specification correct. But that doesn't make really sense. I would prefer two alternatives:

• If it is crucial, that your library is sorted in some way, you could choose to always have a sorted collection of books

• otherwise, I would generate a sorted List on the fly The implementation depends on the usage.

And what is that?

public String toString() {
    return Library.this.toString();
}

The specification was for once clear at this point:

returns a properly formatted String representation of all the books in the library (Title followed by authors).

public String toString() {
    StringBuilder sb=new StringBuilder();
    for(Book b:books) sb.append(b.toString());
    return sb.toString();
}

That should do the trick.

Let's talk about Book:

public Book(String title, ArrayList<String> authors) {
    if (title == null && authors == null) {
        throw new IllegalArgumentException("Can't be null");
    }
    if (title.isEmpty() && authors.isEmpty()) {
        throw new IllegalArgumentException("Can't be empty");
    }
    bookTitle = title;
    bookAuthor = authors;
}

The same as above: Don't throw exceptions in a constructor! Additionally: If you do not allow blank books, why check for blank books in the Library? Are you afraid of malicious subclasses of books, which allow blank books?

public int compareTo(Book other) {
    return bookTitle.compareTo(other.bookTitle);
}

Why only compare the title? There are at least two books calledFrench Kitchen, but they are not the same, since the authors differ.

public ArrayList<String> getAuthors() {
    return bookAuthor;
}

This is naughty and could bite you:

public static void main(String[] args) {
    ArrayList<String> authors=new ArrayList<>();
    authors.add("1");
    authors.add("2");
    authors.add("3");
    authors.add("4");
    Book b=new Book("test", authors);
    ArrayList<String> list1=b.getAuthors();
    list1.add("5");
    ArrayList<String> list2=b.getAuthors();
    for(String s:list2){
        System.out.println(s);
    }
}

You are leaking the reference of your internal authors.

public List<String> getAuthors() {
    List<String> result=new ArrayList<>();
    for(String a:bookAuthor) result.add(a);
    return result;
}

This is what you want.