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
– Prathap, Commented Feb 8, 2017 at 18:02

VHS · Accepted Answer · 2017-02-08 18:10:46Z

1

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;


}

edited Feb 8, 2017 at 18:10

answered Feb 8, 2017 at 18:04

VHS

10.2k3 gold badges25 silver badges48 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

tucuxi Over a year ago

Note that, although this fixes the warning, the code is still broken: if personA.size() >= personB.size(), it returns an empty list of books...

suliman E Over a year ago

Thank you, I appreciate your help.

Community · Accepted Answer · 2017-05-23 11:53:27Z

1

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.

edited May 23, 2017 at 11:53

CommunityBot

11 silver badge

answered Feb 8, 2017 at 18:19

tucuxi

18k2 gold badges48 silver badges80 bronze badges

2 Comments

suliman E Over a year ago

Thanks, but that's a little too advanced for my project.

tucuxi Over a year ago

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.

Community · Accepted Answer · 2020-06-20 09:12:55Z

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, 2020 at 9:12

CommunityBot

11 silver badge

answered Feb 8, 2017 at 18:15

Adilson Cabral

6346 silver badges12 bronze badges

1 Comment

tucuxi Over a year ago

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"

flakes · Accepted Answer · 2017-02-08 18:22:57Z

0

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;

edited Feb 8, 2017 at 18:22

answered Feb 8, 2017 at 18:10

flakes

24.1k8 gold badges52 silver badges101 bronze badges

Comments

Andreas · Accepted Answer · 2017-02-08 18:46:43Z

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 8, 2017 at 18:46

answered Feb 8, 2017 at 18:19

Andreas

5,1562 gold badges28 silver badges37 bronze badges

2 Comments

tucuxi Over a year ago

OP can already be assumed to have a working equals (otherwise, contains() would be failing).

Andreas Over a year ago

Great, I'll cut that out of the answer.

Collectives™ on Stack Overflow

Java 8 ArrayList equality

5 Answers 5

2 Comments

2 Comments

1 Comment

Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

2 Comments

2 Comments

1 Comment

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related