I see once in a while methods like the following one that compare 2 hash maps:
public boolean verifyMapsAreEqual(HashMap<String, String> map1,
HashMap<String, String> map2) {
boolean result = false;
try {
for (String key : map1.keySet()) {
if (map2.keySet().contains(key)) {
String value1 = map1.get(key).trim();
String value2 = map2.get(key).trim();
if (value2.equalsIgnoreCase(value1)) {
result = true;
System.out.println(key + " - " +
value2 + " == " + value1);
}
else
fail(key + " - " + value2 + " != " + value1);
}
else
fail(key + " does not exist!");
}
}
catch (Exception e) {
e.printStackTrace();
}
return result;
}
The method is very straightforward:
it gets the 2 hash maps as parameters
it loops through the keyset of the first map
for each key
it checks if the key is included in the second map
if the key is not included in the second map, the execution is stopped using a fail assertion
if the key is included in the second map, it compares the value that corresponds to the key from the first map to the value that corresponds to the key from the second map
if the values are equal, a message is displayed in the console
if the values are not equal, the execution is stopped using a fail assertion
the method returns true if the maps are equal
There are multiple things that can be improved in this method.
Since the method returns a boolean value, its name should use the “is” or “are” prefix. areEqual() is a much better name than verifyMapsAreEqual().
The method ends in 2 different ways. If the maps are equal, the method returns true. If the maps are not equal, the method ends because of the fail() assertion. We should return a value in all cases instead of using assertions
The method uses a try/catch in case that an exception is generated. If this happens, the exception is not managed at all. The exception info is displayed in the console, nothing else happening in the catch clause. We do not need this try/catch at all
There is no benefit for displaying information in the console when 2 elements are equal
There is no need to use if/else to compare keys or values; just using an if for the negative cases is sufficient
The changed method is below:
public boolean areEqual(HashMap<String, String> map1,
HashMap<String, String> map2) {
boolean result = true;
for (String key : map1.keySet()) {
if (!map2.keySet().contains(key)) {
result = false;
break;
}
String value1 = map1.get(key).trim();
String value2 = map2.get(key).trim();
if (!value2.equalsIgnoreCase(value1) {
result = false;
break;
}
}
return result;
}
The question that still needs to be answered is why is this method needed.
Do we need it?
Can’t we find a class in the Java JDK that can help with comparing 2 maps?
It turns out that the AbstractMap class has an equals() method that can be used for comparing 2 hash maps.
So, instead of using our custom method, we can simply use map1.equals(map2).
What we should do every time we are in a similar scenario, before starting to implement our own methods for common things like comparing 2 maps, is to
question if the Java JDK already has a class and method that does this
question if there are other libraries that have a class and method that does this
Only if we cannot find anything in the JAVA JDK or in another library, we can start implementing our own custom methods.
Because, why should we re-implement the Java JDK instead of using it as is?