Java 8 ArrayList equality

Question

I am trying to use two lists of books two different people have read and compare them. If the a Book( which is a constructor made up of a String title and String author) in the first list is equal to one of the books in the other list then I add it to a commonBooks list. This is what I got so far but I keep getting this suggestion: "add return statement" or "change to void"

here's what I have so far:

public static ArrayList<Book> commonBooks(Person a, Person b) {

    ArrayList<Book> personA = a.getRead();
    ArrayList<Book> personB = b.getRead();
    ArrayList<Book> sameBooks = new ArrayList<Book>();

    if (personA.size() < personB.size()) {
        for (int i = 0; i < personA.size(); i++) {
            Book ndx = personA.get(i);
            if (personB.contains(ndx)) {
                sameBooks.add(ndx);
            } else if (personB.size() < personA.size()) {
                for (int i1 = 0; i1 < personB.size(); i1++) {
                    Book ndx1 = personB.get(i1);
                    if (personB.contains(ndx1)) {
                        sameBooks.add(ndx1);
                    } else if (personB.size() < personA.size()) {
                        for (int i2 = 0; i1 < personB.size(); i2++) {
                            Book ndx2 = personB.get(i2);
                            if (personB.contains(ndx2)) {
                                sameBooks.add(ndx2);
                            }

                        }
                    } else {
                        return sameBooks;
                    }

                }
            }
        }
    } else {
        return sameBooks;
    }

}

Every condition in your program will need to return a value. Else you need to return one in the end outside the conditional blocks. This is just a way of ensuring a value is always returned regardless of the condition met. This applies for a method which expects a value to be returned. Or else remove all return statements and mark the method as void as suggested by the compiler :) — Prathap, Feb 08 '17 at 18:02

VHS · Accepted Answer · 2017-02-08T18:10:46.317

This is what I got so far but I keep getting this suggestion: "add return statement" or "change to void"

Not all the logical flows in your method return the specified return type (ArrayList<Book>) in your method signature. In your if statement, there are some flows which you need to correct to return.

public static ArrayList<Book> commonBooks(Person a, Person b) {

ArrayList<Book> personA = a.getRead();
ArrayList<Book> personB = b.getRead();
ArrayList<Book> sameBooks = new ArrayList<Book>();

if (personA.size() < personB.size()) {
    for (int i = 0; i < personA.size(); i++) {
        Book ndx = personA.get(i);
        if (personB.contains(ndx)) {
            sameBooks.add(ndx);
        } else if (personB.size() < personA.size()) {
            for (int i1 = 0; i1 < personB.size(); i1++) {
                Book ndx1 = personB.get(i1);
                if (personB.contains(ndx1)) {
                    sameBooks.add(ndx1);
                } else if (personB.size() < personA.size()) {
                    for (int i2 = 0; i1 < personB.size(); i2++) {
                        Book ndx2 = personB.get(i2);
                        if (personB.contains(ndx2)) {
                            sameBooks.add(ndx2);
                        }

                    }
                } else {
                    //return sameBooks;
                    break;
                }

            }
        }
    }
} 

return sameBooks;


}

Note that, although this fixes the warning, the code is still broken: if personA.size() >= personB.size(), it returns an empty list of books... — tucuxi, Feb 08 '17 at 18:23

score 1 · Answer 2 · edited May 23 '17 at 11:53

If your method declares that an ArrayList is returned, then all possible paths through your code must return an ArrayList. VHS's answer correctly points out a fix: exit in only one place, which returns an ArrayList, and that all paths must pass through.

Another fix, which goes beyond your original question, is to use simpler code:

HashSet<Book> readByA = new HashSet<>(a.getRead());
readByA.retainAll(b.getRead()); // remove all books that b has not read
return new ArrayList<>(readByA);

Using HashSets requires you to have implemented equals() and hashCode() methods in your Book class, though. Many IDEs will offer to implement them for you. On the other hand, for large numbers of books, this will be much, much faster than your current code.

The original code also has at least one major bug, as it returns an empty list if personA.size() >= personB.size(). So, for example, given two identical lists, it will currently return no books as being common to both!.

Rewriting it again, this time without sets:

ArrayList<Book> readByB = b.getRead();
ArrayList<Book> sameBooks = new ArrayList<Book>();
for (Book b : a.getRead()) {
    if (readByB.contains(b)) sameBooks.add(b);
}
return sameBooks;

Note that, all things being equal, shorter code is easier to read and understand than longer code fragments. That is one reason to prefer...

for (Book b : a.getRead()) {

...to...

ArrayList<Book> readByA = a.getRead();
// ...
for (int i = 0; i < readByA.size(); i++) {
   Book b = readByA.get(i);

...you save 2 lines (66%!), and are no longer confused by 2 auxiliary variables, i and readByA that are not really needed any more.

I added some more code, this time without using sets. Note that your current code, even with VHS's fix, is both overly complex and produces wrong results for many inputs. — tucuxi, Feb 08 '17 at 18:34

flakes · Answer 3 · 2017-02-08T18:22:57.403

You're overthinking this!

Say you have a collection [a, b, c] and another collection [b, c, d].

You only need to iterate over one collection and compare it to the other. eg:

is 'a' in [b, c, d] ? no don't add a
is 'b' in [b, c, d] ? yes add b
is 'c' in [b, c, d] ? yes add c
Result: [b, c]

Notice that you didn't need to loop over the second set? You never evaluated d but you already know it's not in the first list. You checked every item in the first list already so there's no way 'd' will match against [a, b, c].

for (int i = 0; i < personA.size(); i++) {
    Book ndx = personA.get(i);
    if (personB.contains(ndx)) {
        sameBooks.add(ndx);
    }
}
return sameBooks;

score 0 · Answer 4 · edited Jun 20 '20 at 09:12

0

see Java Compare Two Lists

You can try intersection() and subtract() methods from CollectionUtils.

intersection() method gives you a collection containing common elements and the subtract() method gives you all the uncommon ones.

They should also take care of similar elements

I believe your Book class should implement Comparator interface so you can use this properly.

edited Jun 20 '20 at 09:12

Community

1
1

answered Feb 08 '17 at 18:15

Adilson Cabral

548
5
12

1

It is simpler to use sets and `retainAll` - no external dependencies required. You are also not answering OP's original question, "what is wrong in my code" – tucuxi Feb 08 '17 at 18:21

Andreas · Answer 5 · 2017-02-08T18:46:43.273

0

Here is a java 8 streaming solution:

import java.util.List;
import java.util.stream.Collectors;

class Class {
  public static List<Book> commonBooks(Person a, Person b) {
    List<Book> personA = a.getRead();
    List<Book> personB = b.getRead();
    return personA.stream()
      .filter(personB::contains)
      .collect(Collectors.toList());
  }
}

edited Feb 08 '17 at 18:46

answered Feb 08 '17 at 18:19

Andreas

4,937
2
25
35

OP can already be assumed to have a working equals (otherwise, contains() would be failing). – tucuxi Feb 08 '17 at 18:36
Great, I'll cut that out of the answer. – Andreas Feb 08 '17 at 18:44

Java 8 ArrayList equality

5 Answers5