Book program with arrayList

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 allBook = new ArrayList();

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

public L

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);
}

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 allBook = new ArrayList ();

Library () {}

Library (ArrayList 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 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.

StackExchange Code Review Q#33148, answer score: 6