6
\$\begingroup\$

How can I optimize this code or what are the other ways using which I can get length of non-repeated numbers in an array ?

Thanks in advance!

package com.sujay.practice;

import java.util.HashMap;
import java.util.Map;

/**
 * Find the length of non-repeated numbers in an array
 * @author Sujay
 * 08-May-2018
 */
public class DuplicateValuesInArray {

    public static void main(String[] args) {
        int[] array = { 1, 2, 2, 3, 4, 5, 6, 2, 4, 5, 3 };
        usingMap(array);
        usingString(array);
    }

    private static void usingString(int[] array) {
        String str = "";
        for (int i = 0; i < array.length; i++) {
            String temp = String.valueOf(array[i]);
            if (!str.contains(temp)) {
                str = str + temp;
            }
        }
        System.out.println(str.length());
    }

    private static void usingMap(int[] array) {
        Map<Integer, Integer> map = new HashMap<Integer, Integer>();
        for (int i = 0; i < array.length; i++) {
            map.putIfAbsent(array[i], i);
        }
        System.out.println(map.size());
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis May 8 '18 at 18:03
6
\$\begingroup\$
  • The correct data structure to use to eliminate duplicates is a Set. The String processing is more expensive, and the Map code is less clear because you don't care at all about the values.

  • It's nice to provide a vararg parameter so that clients don't need to define an array if they don't want to.

  • It's a better practice to return your result from the method. If the client wants to print it, that's up to them. But if you print it, and the client changes what they want to do with it, they're out of luck.

  • You can used the enhanced for loop to make your code a little prettier.

  • More descriptive names are better than less descriptive names.

If I were to take a stab at writing your code with the above changes in mind, it might look something like:

private static int countUniqueValues(final int... values) {
    final Set<Integer> uniqueValues = new HashSet<>(values.length);
    for (final int value : values) {
        uniqueValues.add(value);
    }
    return uniqueValues.size();
}
\$\endgroup\$
1
\$\begingroup\$

Your first version is unreliable if there are numbers that consist of more than one digit. For example, the method usingString(int[]) would count only two distinct numbers in the array [1, 2, 12], and only one distinct number in the array [12, 1, 2].

Apart from that, you could also use a stream:

static int countDistinctIntegers(int[] array) {
    return (int) Arrays.stream(array).distinct().count();
}

I think this better represents the intention behind the code than using a Map (or a Set, which, I agree, is more appropriate than a Map here), because collecting the elements to a Collection is not an aim per se, but just a means to accomplish something else, whereas the stream describes exactly what you want to do, seeing as streams are meant to represent operations on data as opposed to storage of data.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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