Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
    }
}
share|improve this question
    
Post rolled back. Please do not make changes to code based on answers as that will invalidate them. You may instead post the new code below the original. –  Jamal Oct 24 '13 at 7:15

4 Answers 4

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.

share|improve this answer
    
Thank u so much, I will try that asap. –  Bopeng Liu Oct 24 '13 at 3:15

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!

share|improve this answer
    
I got everything else, But still not sure how the toString method in Library works. –  Bopeng Liu Oct 24 '13 at 10:32
    
In C# I'd use a StringBuilder and builder.Append(thisBook.ToString()); on each iteration, and then I'd return builder.ToString(). –  Mat's Mug Oct 24 '13 at 12:41

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

share|improve this answer

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.

share|improve this answer
    
thank you, i end up using the for each method,I think I got the problem figured out. Now im working on the test class which is public static void main(String[] args). –  Bopeng Liu Oct 24 '13 at 7:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.