Need assistance reducing duplicate code

Question

I'm a beginner and the lecturer told me to reduce the duplicate code in these two functions. This is a library. I guess I need one more method for checking through the array. All I need is some advice/principle I need to use. I'll write the method myself. Thanks! getBooks() returns the ArrayList where Books are stored.

public List<Book> getAvailableBooks() {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if (this.getBooks().get(i).getPerson() == null) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

public List<Book> getUnavailableBooks() {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if (this.getBooks().get(i).getPerson() != null) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

Answer 1

You already have two methods. You can't reduce by adding one more.

But you can have one method instead of two. e.g.

public List<Book> getBooks(boolean available) {

Now you have one method, and you can tell it whether you want available or unavailable books.

Answer 2

Pass in a boolean parameter that should indicate what

(this.getBooks().get(i).getPerson() == null)

Should evaluate to, and add a condition to check that. I.e. should this expression return true or false.

Answer 3

Here're some advice:

• Use enhanced for loop instead of indexed one. This will avoid using this.getBooks().get(i) twice in each method.

• Unavailable books + Available books = Total books. Use this equation to avoid writing all the codes in both the methods. You might want to use a Set<Book> instead of List<Book> to make this easier to work with. [HINT: Set Difference].

• Also, rather than doing the null check in those methods, I'll add a method isAvailable() inside the Book class only, which will return true if it is available, else false.

Answer 4

In the general case, if you see a very common pattern repeated in your code, there is often an opportunity to reduce duplication. The first step is to look at your code and identify the actual differences. You have:

public List<Book> getAvailableBooks() {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if (this.getBooks().get(i).getPerson() == null) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

public List<Book> getUnavailableBooks() {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if (this.getBooks().get(i).getPerson() != null) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

Ignoring the method names, let's do a line-by-line comparison. Doing this, we can spot the difference:

if (this.getBooks().get(i).getPerson() == null) {
// vs:
if (this.getBooks().get(i).getPerson() != null) {

The only difference there is == vs. !=; the condition is inverted. After identifying the differences, the next step is to see if you can parameterize the behavior, so that the logic itself is exactly the same and depends only on the value of a few outside variables. In this case, we can see a transformation like this:

a == x    =>    (a == x) == true
a != x    =>    (a == x) == false
both:     =>    (a == x) == <variable>

So we can make those two lines equivalent:

boolean available = true;
if ((this.getBooks().get(i).getPerson() == null) == available) {
// vs:
boolean available = false;
if ((this.getBooks().get(i).getPerson() == null) == available) {

Now the logic is equivalent, and we can select between the two by simply changing available. Make that a method parameter and, voila!

public List<Book> getBooksByAvailability (bool available) {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if ((this.getBooks().get(i).getPerson() == null) == available) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

Note that another way to approach this problem is to make "availability" itself be a property of a book. For example, if you move the availability test into a new method Book.isAvailable(), the solution becomes a bit more obvious:

public List<Book> getBooksByAvailability (bool available) {
    List<Book> result = new ArrayList<Book>();
    for (int i = 0; i < this.getBooks().size(); i++) {
        if (this.getBooks().get(i).isAvailable() == available) {
            result.add(this.getBooks().get(i));
        }
    }
    return result;
}

And that has the added bonus of letting you change the internal definition of "availabilty" without modifying code anywhere else (e.g. if, in the future, you decide that getPerson() == null is not a sufficient indication of "availability", you only need to change it in Book.isAvailable()).

As for clarity, you could, as Rohit Jain mentioned in this answer, switch to enhanced for loops to improve readability a bit, e.g.:

public List<Book> getBooksByAvailability (bool available) {
    List<Book> result = new ArrayList<Book>();
    for (Book book : this.getBooks()) {
        if (book.isAvailable() == available) {
            result.add(book);
        }
    }
    return result;
}

To keep your two existing functions, if that's necessary, you can use something like the above as a private utility method, called by the two public ones.

Answer 5

If you don't want to change the method signature, I think you cannot do much better than you already have. You can just rewrite the loops to foreach and possibly do the substractions of the lists. Dirty solution, but the lecturer may take it.

public List<Book> getAvailableBooks() {
    Set<Book> result = new HashSet<>(getBooks());
    result.removeAll(getUnavailableBooks());
    return new ArrayList<Book>(result);
}

public List<Book> getUnavailableBooks() {
    List<Book> result = new ArrayList<Book>();
    for (Book b: getBooks()) {
        if (b.getPerson() != null) {
            result.add(b);
        }
    }
    return result;
}

Probably not the fastest solution, but quite short