4
\$\begingroup\$

This is my first post. I'm switching from Python to Java and I've been practicing on HackerRank for some time. Today I solved this problem in which you have to count the pairs of occurrences of the numbers in the ArrayList, symbolizing socks, and you have to count the pairs.

Can this be optimized in any way? Maybe I'm too used to Dictionaries' methods and syntax in Python. Anyway, any feedback is appreciated.

public static int countPairs() {

    HashMap<Integer,Integer> mapa = new HashMap<Integer,Integer>();
    ArrayList<Integer> list=new ArrayList<Integer>();

    list.add(10);
    list.add(20);
    list.add(20);
    list.add(10);
    list.add(10);
    list.add(30);
    list.add(50);
    list.add(10);
    list.add(20);

    int pairs=0;
    for (int i = 0; i < list.size(); i++) {
        if (!mapa.containsKey(list.get(i))) {
            mapa.put(list.get(i), 1);
        } else {
            mapa.replace(list.get(i), mapa.get(list.get(i))+1);
        }
    }

    int whole;
    for (Integer key : mapa.keySet()) {
        whole = (int)mapa.get(key)/2;
        pairs=pairs+whole;
    }
    return pairs;
}
\$\endgroup\$
0
4
\$\begingroup\$

Prefer assigning variables to the most generally applicable type. If it doesn't matter what kind of list you have, use List. If it doesn't matter what kind of map you have, use Map.

Use whitespace consistently. =, +, and / should have whitespace on both sides.

Code doesn't need to declare the generic type on the right hand side of an assignment. The compiler will infer it from what is declared on the left hand side. Use <> instead.

Arrays.asList() is probably easier to read than adding multiple values to the list. Do note that this type of list is fixed size after it is created.

list is not very descriptive, nor is mapa. Descriptive names are very important.

It would be preferable for the code to use an enhanced for loop. Iterators are more efficient at going over some List types than calling get(). They are also easier to read.

The if-else block could be replaced with a getOrDefault() call followed by a put.

It would be preferable to iterate over the mapa values instead of keys. The keys don't matter except to separate out the counts.

The declaration of pairs belongs closer to where it's actually used.

whole can be declared inside the loop.

All the math can be done on one line inside the second loop.

The logic can also be done more efficiently using a Set. Iterate over each sock. If it's in the set, remove it and increase the pair count. If it's not in the set, add it.

If you made all these changes, your code might look like:

public static int countPairs() {
    Map<Integer, Integer> sockCounts = new HashMap<>();
    List<Integer> sockIds = Arrays.asList(10, 20, 20, 10, 10, 30, 50, 10, 20);

    for (int sockId : sockIds) {
        int currentCount = sockCounts.getOrDefault(sockId, 0);
        sockCounts.put(sockId, currentCount + 1);
    }

    int pairs = 0;
    for (int count : sockCounts.values()) {
        pairs += (count / 2);
    }
    return pairs;
}

public static int countPairsWithSet() {
    List<Integer> sockIds = Arrays.asList(10, 20, 20, 10, 10, 30, 50, 10, 20);
    Set<Integer> unmatchedSocks = new HashSet<>();

    int pairs = 0;
    for (int sockId : sockIds) {
        if (unmatchedSocks.contains(sockId)) {
            unmatchedSocks.remove(sockId);
            pairs++;
        } else {
            unmatchedSocks.add(sockId);
        }
    }
    return pairs;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You can also take advantage of the return value of add to optimize runtime performance slightly at the cost of readability: if (!unmatchedSocks.add(sockId)) { unmatchedSocks.remove(sockId); pairs++; } Experienced developers will recognize that, but it will probably confuse junior developers. \$\endgroup\$
    – Eric Stein
    Jun 26 '21 at 21:08
4
\$\begingroup\$

I noticed in your code the following definitions:

HashMap<Integer,Integer> mapa = new HashMap<Integer,Integer>();
ArrayList<Integer> list=new ArrayList<Integer>();

When it is possible it is better to use an interface than a class implementing the interface like below:

Map<Integer,Integer> mapa = new HashMap<Integer,Integer>();
List<Integer> list=new ArrayList<Integer>();

So for example if you want to use TreeMap instead of HashMap you can just declare Map<Integer,Integer> mapa = new TreeMap<Integer,Integer>(); without affecting the rest of the codebase.

In the line whole = (int)mapa.get(key)/2; you can erase the cast to int, I think you added this because related to your Python programming practice.

\$\endgroup\$
3
\$\begingroup\$

In your for-loop, you basically mimic Map.computeIfAbsent. For a given int n, you can use

map.computeIfAbsent(n, ignore -> 0);

to return 0 when the key is not present, and thus

map.put(n, map.computeIfAbsent(n, ignore -> 0) + 1);

as your calculation.

Note that ignore -> 0 is a lambda of the signature Function<Integer, Integer>, which does not use its input parameter. Therefore I named it "ignore".

\$\endgroup\$
2
  • \$\begingroup\$ Not sure if this is allowed in java, but the traditional name for a lambda parameter you ignore is _, as in _ -> 0 \$\endgroup\$ Jun 26 '21 at 11:52
  • \$\begingroup\$ Use putIfAbsent to avoid the lambda. Or, better, getOrDefault as the returned value is put in the map anyway. \$\endgroup\$
    – Édouard
    Jun 26 '21 at 18:45
0
\$\begingroup\$

In Java 8 introduced Streams: have a look at them! Despite a somewhat verbose syntax, you might find things you're more familiar with in Python.

You'll probably be interested in the groupingBy Collector. And in Map#replaceAll.

\$\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.