Why does the method not compute maximum greater than 65537?

Question

public int longestConsecutive(int[] nums) {
        Map<Integer, Boolean> numMap = new ConcurrentHashMap<>();
        Map<Integer, Integer> maxMap = new ConcurrentHashMap<>();
        for (int i : nums) {
            numMap.put(i, false);
        }
        int max = 0;
        for (int n : numMap.keySet()) {
            numMap.remove(n);

            if (maxMap.containsKey(n - 1)) {
                maxMap.put(n, maxMap.get(n - 1) + 1);
                max = Math.max(maxMap.get(n), max);
                continue;
            }

            int lessThan = 0;
            while (numMap.containsKey(n - lessThan - 1)) {
                numMap.remove(n - 1);
                lessThan++;
            }
            maxMap.put(n, lessThan + 1);
            if (lessThan + 1 > max) {
                max = lessThan + 1;
            }

        }
        return max;
    }

This is my solution to Longest Consecutive Sequence problem and it works for all but 1 of the 70 cases. The 70th case array has length 100,000. I checked my solution for this and other lengths of the array and I observed that the max value returned is always 65537 for arrays of size greater than 65537. I can't seem to figure out why that is the case. I wonder if it has something to do with using ConcurrentHashmap.

Here's my test:

  @Test
    public void test() {

        for (int i = 0; i < 100000; i++) {
            int n = i;
            int[] arr = new int[n];
            for (int j = 0; j < n; j++) {
                arr[j] = j;
            }
            assertEquals(n, longestConsecutive(arr));
        }

    }

The test fails at 65538 by returning 65537. And I also checked for some random values greater than that and it failed for them too with the same value.

Why are you using a map for the `numMap`? You don't really use the value, do you? Why not use a BitSet instead? — RealSkeptic, Jan 30 '22 at 13:13
@RealSkeptic I cannot use sets because of the ConcurrentModificationException — , Jan 30 '22 at 13:14
@GoldCredential Don't modify as you loop on them. You can simply use a HashSet for this as we don't care about the frequency of an element. — nice_dev, Jan 30 '22 at 13:24
`numMap.remove(n - 1)` looks like a bug. Also, better to iterate through `nums` again instead of `numMap.keySet`, then just check to see if it's in the map. Then you can use a HashMap, because ConcurrentHashMap is slow and wastes a lot of memory. — Matt Timmermans, Jan 30 '22 at 13:33

DuncG · Answer 1 · 2022-01-30T14:10:44.523

ConcurrentModificationException means your logic is wrong - you are editing and iterating a map at the same time.

The iteration numMap.keySet() is the values of nums - you should be OK to change the outer loop to:

for (int n : nums) {
    ...
}

This avoids ConcurrentModificationException on the iterator while edits are performed. You don't need to use ConcurrentHashMap over HashMap - both should work fine in this situation, and the tests pass for 100,000.

Why does the method not compute maximum greater than 65537?

1 Answers1