CODEWITHSUNDEEP
javalist
David
David

Reputation: 521

Use method to compare lists

I'm trying to use a method to compare t2o different lists. Basically I want to pass two different lists to a method which will return true or false if the elements of one array list are contained in the other using .contains. Right now it only returns true - and I'm not sure why. I'd like it to return false. If someone could help me figure this out, that would be great.

public class ArrayListTest {

public static void main(String[] args) {

    List<String> list1 = new ArrayList<String>();
    List<String> list2 = new ArrayList<String>();

    list1.add("cat");
    list1.add("dog");
    list1.add("zebra");
    list1.add("lion");
    list1.add("mouse");

    //Test Values
    //list2.add("cat");
    list2.add("lizard");

    boolean doesitcontain = contains(list1, list2); 
    System.out.println(doesitcontain);


}

public static boolean contains (List<String>list1, List<String>list2){

boolean yesitcontains;

for(int i = 0; i < list1.size(); i++){

    if(list2.contains(list1.get(i))){
        System.out.println("Duplicate: "+list1.get(i));
        yesitcontains = true;
        System.out.println(yesitcontains);
    }else{
        yesitcontains = false;
        System.out.println(yesitcontains);
    }
}
    if (yesitcontains = true){

        return true;

    }else 

        return false;

}

}

Upvotes: 0

Views: 212

Answers (4)

Marko Topolnik
Marko Topolnik

Reputation: 200296

  1. You have inadvertently used the assignment operator where you intended the equality operator. In your specific case you should rewrite all this:

    if (yesitcontains = true){

    return true;

}else 

   return false;

}

    to just

    return yesitcontains;

    and avoid any chance of confusion.

  2. Furthermore, your algorithm will not work because you should return true immediately when you see a duplicate. Instead you go on with the loop and "forget" your finding. You can expect this to always return false except if the very last elements coincide.

  3. In a wider context, I should also give you the following general advice:

    • Avoid indexed iteration over lists. Not all lists are ArrayLists and may show O(n) complexity for get(i). Instead use the enhanced for loop, which is safer, more concise, and more obvious;
    • Know the library: if you're just after confirming there are no duplicates, just Collections.disjoint(list1, list2) would give you what you need;
    • Be aware of algorithmic complexity: checking for duplicates in two lists is O(n2), but if you turn one of them into a HashSet, you'll get O(n).

Taking everything said above into account, the following would be an appropriate implementation:

static boolean disjoint(Collection<?> c1, Collection<?> c2) {
    for(Object o : c1) 
      if (c2.contains(o)) 
        return true;
    return false;
}

If you look at Collections.disjoint, you'll find this exact same loop, preceded by a piece of code which optimizes the usage of sets for reasons described above.

Upvotes: 6

jlordo
jlordo

Reputation: 37843

Seems to me your method should be rewritten to:

public static boolean contains(List<String>list1, List<String>list2) {
    return list2.containsAll(list1);
}

The code you currently have actually only checks if the last element of list1 is also in list2.

If you're actually looking for a contains any, this simple solution will do:

public static boolean contains(List<String>list1, List<String>list2) {
    for (String str : list1) {
        if (list2.contains(str)) {
            return true;
        }
    }
    return false;
}

Upvotes: 3

kosa
kosa

Reputation: 66667

 if (yesitcontains = true){

should be

if (yesitcontains == true){

== is for comparison and = is for assignment.

if (yesitcontains = true){

will always evaluate to if(true) which causing return true;

EDIT:

(OR)

simply return yesitcontains; as commented.

Upvotes: 3

AllTooSir
AllTooSir

Reputation: 49432

if (yesitcontains == true)  { } // use `==` here

or just 

if (yesitcontains) { }

The below code assigns true to yesitcontains , and the expression will always be true.

if (yesitcontains = true) { }

There is no point of if() in your code , you can simple return yesitcontains;

Upvotes: 1

Related Questions