CODEWITHSUNDEEP
javaarraylist
suliman E
suliman E

Reputation: 23

Java 8 ArrayList equality

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

}

Upvotes: 2

Views: 787

Answers (5)

Andreas
Andreas

Reputation: 5103

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

Upvotes: 0

tucuxi
tucuxi

Reputation: 17955

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.

Upvotes: 1

flakes
flakes

Reputation: 23674

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;

Upvotes: 0

Adilson Cabral
Adilson Cabral

Reputation: 628

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.

Upvotes: 0

VHS
VHS

Reputation: 10184

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;


}

Upvotes: 1

Related Questions