-1

I am trying to find items that occur in two sets. I have to use Sets, that is why I am not using any other libraries, etc.

My code is the following:

public static void intersection (ArrayList<Integer>s1, ArrayList<Integer>s2) {
      HashSet <Integer> all = new HashSet<Integer>();
      HashSet <Integer> both = new HashSet<Integer>();

            for (int i=0; i<s1.size(); i++)
                all.add(s1.get(i));

            for (int x=0; x<s2.size(); x++) {
                if ((!(all.add(s2.get(x)))) && (((all.contains(s2.get(x)))))) {
                    both.add(s2.get(x));
                        }
            }
            System.out.println("intersection - "+ both);
}

The arraylists contain the following values:

s1: 4 5 5 6 76 7 7 8 8 8 8 8

s2: 23 3 4 3 5 3 53 5 46 46 4 6 5 3 4

However, the output is the following:

3, 4, 5, 6, 46

My desire output is:

4, 5, 6

I understand that it is adding 3 and 46 because to the ArraySet both because those are elements present in the s2 ArrayList but not in s1. However, I added all.contains(s2.get(x)) to make sure that the number added is present in s1 or HashSet all. Why is not working?

M. A. Kishawy
  • 5,001
  • 11
  • 47
  • 72
ZeldaX
  • 49
  • 9
  • it's for an assignment so I have to write like that.. @ReutSharabani – ZeldaX Oct 07 '16 at 15:08
  • 1
    have a look at retainAll() or just have a look here: http://stackoverflow.com/questions/8882097/is-there-a-way-to-calculate-the-intersection-of-two-sets – hasan Oct 07 '16 at 15:11
  • please format your code properly. Some comments: a) you can re-use `int i` in the second `for`-loop, b) why do you add everything from `s1` to `all`? As far as I understand, you only need the intersection, not the union. – Turing85 Oct 07 '16 at 15:14
  • There's nothing wrong per se with using HashSet with Integers. But with `Comparable` values, using a TreeSet has its advantages (and disadvantages) – ControlAltDel Oct 07 '16 at 15:16

3 Answers3

3

How about this

public static void intersection (ArrayList<Integer>s1, ArrayList<Integer>s2)
{
    HashSet <Integer> intersect = new HashSet<Integer>();

    for (int i=0; i<s1.size(); i++)
    {
        if (s2.contains(s1.get(i))) intersect.add(s1.get(i));

    }

    System.out.println("intersection - "+ intersect);

}
Hendrik T
  • 191
  • 7
2

Your program could be reduced to

Set<Integer> both = new HashSet<>(s1);
both.retainAll(s2);
System.out.println("intersection - " + both);
JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
0

Simply put, checking for presence in the all list like you are doing here is wrong, as you could have already added this value off of the s2 list.

 if ((!(all.add(s2.get(x)))) && (((all.contains(s2.get(x)))))) {

You need to check it against the contents of s1. The question is whether you just want to call

 s1.contains (s2.get(x))

or use a possibly more efficient solution like converting s1 to a TreeSet first, or sorting s1 and doing a binary search

ControlAltDel
  • 33,923
  • 10
  • 53
  • 80