1
\$\begingroup\$

I implemented a solution for: Finding the most frequent number from an array. How are my variable names? Or how could I improve this?

 private static Integer findMostFrequentNumber(int[] givenArray) {
    int mostFrequentNumber = 0;
    if (givenArray != null) {
        Map<Integer, Integer> numberOccuranceMap = new HashMap<>();
        int mostFrequentOccurance = 0;

        for (Integer number : givenArray) {
            if (!numberOccuranceMap.containsKey(number)) {
                numberOccuranceMap.put(number, 1);
            } else {
                numberOccuranceMap.put(number, numberOccuranceMap.get(number) + 1);
            }
            if (numberOccuranceMap.get(number) > mostFrequentOccurance) {
                mostFrequentNumber = number;
                mostFrequentOccurance = numberOccuranceMap.get(number);
            }
        }
    }
    return mostFrequentNumber;
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Please add a /** */ javadoc sentence, clarifying what happens in e.g. the case where { 5, 5, 6, 6 } is passed in.

The name findMostFrequentNumber() seems a bit verbose, when mode() or findMode() would suffice.

Consider numbers instead of givenArray. In any event, "array" is redundant with int[].

Consider eliding the null check. Caller gets what he deserves if the contract is "caller shall provide an array of numbers". NPE is appropriate if caller fails to do that. Or throw your own exception. Returning zero is clearly inappropriate, as caller cannot distinguish between an input of null and an input of { 5, 6, 0, 0, 0, 5 }.

In numberOccuranceMap, the "map" is redundant with the Map declaration. (Also, typo: occurrence.) Consider a "to" name for your maps, such as numToOccurences or numToCount.

    int mostFrequentOccurance = 0;

This sounds like it is simply modeCount, and the mostFrequentNumber we will return is simply mode.

            numberOccuranceMap.put(number, numberOccuranceMap.get(number) + 1);

Your get() call could specify a 2nd argument default value of zero, which would obviate the need for the containsKey() test.

        if (numberOccuranceMap.get(number) > mostFrequentOccurance) {
            mostFrequentNumber = number;
            mostFrequentOccurance = numberOccuranceMap.get(number);
        }

Here we find the answer to the javadoc question above: last one wins!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.