3
\$\begingroup\$

I've the following code which check for duplicate property and provide and warning when the key is exist and provide error if the key and the value have the same values. The code have done some iteration and its working my question is there is a way to avoid the if's statement's ?

      Map<String, Object> existProp = requires.get(reqidx).getProperties();
        Optional<Map.Entry<String, Object>> foundProperty = properties.entrySet().stream().filter(property -> existProp.containsKey(property.getKey())).findFirst();
        if (foundProperty.isPresent()) {
            // Same key already exists 
            LOG.warn("Property with key {} already exists in requires {}", foundProperty.get().getKey(), requires.get(reqidx).getName());
            // in case the existing property has the same value (check only for value of type String)
            if (foundProperty.get().getValue() instanceof String
                    && existProp.get(foundProperty.get().getKey()) instanceof String
                    && (foundProperty.get().getValue()).equals(existProp.get(foundProperty.get().getKey()))) {
                throw new Exception("Property " + foundProperty.get().getKey() + "  exists already with value " + foundProperty.get().getValue() + " in requires " + requiresName);
            }
        }
        return reqidx;
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Jan 30 '18 at 13:01
  • \$\begingroup\$ This is a follow-up question to that question. \$\endgroup\$ – Raimund Krämer Jan 31 '18 at 8:17
2
\$\begingroup\$

Every Optional.isPresent() can be replaced with the fluent Optional.ifPresent() which takes a Consumer function arg (that can be lambda expr) which accepts the value of the Optional.get()

properties.entrySet().stream().filter(property -> existProp.containsKey(property.getKey()))
.findFirst().ifPresent(foundPropertyValue -> {
  // Same key already exists 
  LOG.warn("Property with key {} already exists in requires {}", foundPropertyValue.getKey(), requires.get(reqidx).getName());
  // in case the existing property has the same value (check only for value of type String)
  if (foundPropertyValue.getValue() instanceof String
      && existProp.get(foundPropertyValue.getKey()) instanceof String
      && (foundPropertyValue.getValue()).equals(existProp.get(foundPropertyValue.getKey()))) {
        throw new Exception("Property " + foundPropertyValue.getKey() + "  exists already with value " + foundPropertyValue.getValue() + " in requires " + requiresName);
  }
});
\$\endgroup\$
  • \$\begingroup\$ That last if is still horrendous, I think that part can also be improved probably (without having looked too closely at it) \$\endgroup\$ – Simon Forsberg Feb 1 '18 at 16:09
1
\$\begingroup\$

basically you have one filtering condition, no reason to split it.
of value being a string, just check it in the function in stream, like:

property -> {
Object val = existProp.get(property.getKey())
return val != null && val.getClass().equals(String.class) && val.equals(property.getValue();
}

Do you need to look for another prop, if the same key was found but the value is not a String?

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