poset iteration implementation correctness

Question

I wrote a custom iterator class that iterates over the set of numbers found in a PoSet, and here is my code:

private class IntGenerator implements Iterator {
        private Iterator<Integer> i;
        private Set<Integer> returnedNumbers;


        public IntGenerator () {
            returnedNumbers = new HashSet<Integer> ();
            i = S.iterator();
        }

        public boolean hasNext() {
            return i.hasNext();
        }

        public Object next() {
            int n = i.next();

            for (Pair p : R) {
                if (isInSecondElmPair(p, n)) {
                    if (returnedNumbers.contains(p.getFirstElm())) {
                        returnedNumbers.add(n);
                        return n;
                    }else{
                        returnedNumbers.add(p.getFirstElm());
                        return p.getFirstElm();
                    }
                }else if (isInFirstElmPair(p, n)){
                    returnedNumbers.add(n);
                    return n;
                }
            }
            return n;
        }

        public void remove() {
            throw new UnsupportedOperationException();
        }
    }

The thing is that when returning a number, I should abide by the partial order rules, that is: 1. if (x, y) belongs to R, then x should be returned before y

However the code above seems to follow that ordering but it is creating duplicates, how can I fix my code to not allow it?

NOTE: In my code, S is the set of numbers in the PoSet, it is a HashSet and R is an arraylist of pairs (pair: a class i created that takes 2 ints as param) to hold the relations in the PoSet.

Is there any way to fix this problem? Thanks

Answer 1

Your next method always calls i.next(), and returns one of two things:

• the value that i.next() returned
• some value that is less than that value.

This means that if your poset contains {1,2,3,4} and uses the natural ordering for integers, and i.next() returns 4, then either you return 4 now (due to 1, 2, and 3 already having been returned), or you will never return 4 (because it's not less than any future value).

The reason you're getting duplicates is that you return one value for every value of i.next(), and there are some values that never get returned (see previous paragraph), so naturally there are some values that get returned multiple times in compensation. Note that you never check whether the value returned from i.next() has previously been returned by your next() method, so if an element in the poset is not greater than any other element, then when i.next() returns that element, your next() method will automatically return it, even if it has previously returned it.

I think the only sensible fix for this to completely change your approach; I don't think your current approach can readily be made to work. I think your iterator's constructor needs to copy all the elements of the poset into an acceptably-ordered list, and then the next() method will simply return the next element of that list. Or, alternatively, since your current approach already requires iterating over R on every call to next() anyway, it might make more sense to base your iterator on an iterator over R. (I'm assuming here that R is already ordered using itself; if it's not, then your for loop makes no sense at all, and will essentially return randomly selected elements.)

If you do want to try to stick with your approach, then you'll need to keep track not only of the elements that your next() method has returned, but also of the elements that i.next() returned but that your next() method did not return; you'll need to be able to return these elements later.

Also, your for (Pair p : R) loop doesn't do what you want — it automatically returns n as soon as it finds any element that is less than n that's already been returned, even if there are other elements less than n that haven't been returned yet. (This is if R is already ordered using itself. If it isn't, then this loop has even bigger problems.)