CODEWITHSUNDEEP
javadictionary
St.Antario
St.Antario

Reputation: 27375

Double checking for existence a map element?

In the code I'm working on right now I came across the following piece:

PartnerReportItem item = (map.get(partnerId) == null) ? null
    : map.get(partnerId).get(platform); //1, check for existence

if (item == null) {
    item = new PartnerReportItem();
    item.setPlatform(platform);
    item.setPartnerId(partnerId);
    item.setPartnerEmail(partner.getEmail());
    item.setPartnerPaymentDetails(paymentDetails);
    item.setPartnerCoef(partner.getCoef());
    item.setExchangeMap(exchangeMap);

    if (!map.containsKey(partnerId)) //2, double check for existence??
        map.put(partnerId, new HashMap<Platform, PartnerReportItem>());
    map.get(partnerId).put(platform, item);
}

I'm confused by //1 and //2 because I think //2 is unneccesary here. We've already checked if the element with partnerId existed in the map. May I not get a hidden idea?

Upvotes: 1

Views: 78

Answers (3)

Alexis C.
Alexis C.

Reputation: 93842

As the other answers stated, you need to keep this check. However this is a bad coding actually, because there are multiple reasons which are unrelated for the PartnerReportItem instance to be null (and hence the second check in the first if statement).

You better have to split it to make the intent more clear (and avoid confusions when others are reading your code).

Map<Platform, PartnerReportItem> innerMap = map.get(partnerId);
if(innerMap == null) {
    innerMap = new HashMap<Platform, PartnerReportItem>();
    innerMap.put(partnerId, createPartnerReportItem(...));
} else {
    PartnerReportItem item = innerMap.get(partnerId);
    if(item == null) {
        innerMap.put(partnerId, createPartnerReportItem(...));
    }
}

or another variant:

Map<Platform, PartnerReportItem> innerMap = map.get(partnerId);
if(innerMap == null) {
    innerMap = new HashMap<Platform, PartnerReportItem>();
}
PartnerReportItem item = innerMap.get(partnerId);
if(item == null) {
    innerMap.put(partnerId, createPartnerReportItem(...));
}

The point is that the programmer who wrote this ternary nested statement though that it would be less code but at the end, you see that it's just confusing. Always write readable code!

Upvotes: 1

Eran
Eran

Reputation: 393771

It looks like you have a map within a map.

PartnerReportItem item = (map.get(partnerId) == null) ? null : map.get(partnerId).get(platform);

If item is not null, this means that partnerId is found in the outer map and platform is found in the inner map.

However, if item is null, it's still possible that partnerId key exists in the outer map, which means the containsKey check if necessary.

Upvotes: 1

Adrian B.
Adrian B.

Reputation: 1570

the item might be null because the map.get did not return anything (the outer map does not exist) or because the inner map does not contain the requested object. Therefor, a second check is performed to detect the actual reason item became null.

Upvotes: 4

Related Questions