1

How can I simplify the following code using lambda java8? I am new to lambda and still learning it.

public boolean isValuePresent(String id, Optional<String> value) {
        ClassConfig config = configStorage.getConfig();
        Map<String, FeatureClass> map = config.getConfigMap();
        if (map.containsKey(id)) {
            Set<String> set = map.get(id).getset();
            if (!set.isEmpty()) {
                if (value.isPresent()) {
                    if (set.contains(value.get())) {
                        log.info(String.format("value present for id %s", id));
                        return true;
                    }
                }
            }
        }
        return false;
    }
2
  • For starters, create a new function for a block if oyu've found you nested more than two if statements. Commented Feb 3, 2017 at 6:48
  • Why would you have an Optional<String> as method input? Surely you can check if it's present beforehand and just assume that "value is not present"? Commented Feb 3, 2017 at 10:49

3 Answers 3

2

You have done external looping and want to use stream API which provide us to concentrate on internal looping logic.

In stream , there are 3 parts. one is source here it is map.keySet(), second is series of intermediate operations i.e filter(), map() etc.., all operation creates new streams and does not change source, third is terminal operation i.e forEach(), collect(), anyMatch(), allMatch() etc...

Stream has consumable and lazy execution properties. That means that untill we use any termination operation, stream are not yet executed. And once termination operator is applied, stream has consumed and cannot consume further.

boolean retval = map.keySet().stream()
         .filter(x -> x.equals(id))
         .filter(x -> !map.get(x).getset().isEmpty())
         .filter(x -> value.isPresent())
         .anyMatch(x -> map.get(x).getset().contains(value.get()));

Collection API from java8 implements stream interface. Map is not part of collection API so that we are getting collection using map.keySet() and applying stream(). This became our source. filter operation takes any boolean expression here we are comparing map keys with id parameter and checking value param is present or not. anyMatch is termination operation, which will return true if any key in map will satisfy the above criteria.

Sign up to request clarification or add additional context in comments.

2 Comments

@Filnor I have added stream explanation as you have suggested.
to print logger we can use peek() operation, which will not consume the stream
0

In short - no. There is nothing wrong or complicated with this code that is solvable with lambdas per se. What you should rather do is review what this code actually does and simplify it from there. My take on it, is if you can not change signature of isValuePresent, then do something like this:

boolean isValuePresent(String id, Optional<String> option) {
  if (!option.isPresent()) { // this already saves us loading the config if we've got no value to check against
    return false;
  }
  FeatureClass features = configStorage.getConfig().getConfigMap().get(id);
  if (features == null) { // replaces containsKey().get() with one call. It is hardly an improvement, just my stylistic choice
    return false;
  }
  String value = option.get();
  if (features.getset().contains(value) { // we actually have no need to check if set is empty or not, we rather have it handle that itself
    log.info(String.format("value present for id %s", id));
    return true;
  }
  return false;
}

Or, if you can change signatures, I think it's better to have something like this:

boolean isValuePresent(String id, String value) {
  FeatureClass features = configStorage.getConfig().getConfigMap().get(id);
  if (features == null) {
    return false;
  }
  if (features.getset().contains(value)) {
    log.info(<snip>);
    return true;
  }
  return false;
}

And call it like this:

String id = loadId();
Optional<String> myValue = loadMyValue(id);
return myValue.map(value -> Checker.isValuePresent(id, value)).orElse(false);

As you can see, there is no lambdas, because there's no need for them - at no point here we are required to change our behavior, there's no strategy in this method that could possibly be plugged in from outside.

Now, about why I chose to not have Optional in argument:

  1. It offers more flexibility to callers of your method. If they have value String form already, they will not be required to wrap it in useless Optional before calling your method.
  2. Using Optional as method argument type looks ugly, because that class is designed for usage as return value, and doesn't offer much as argument type besides isPresent() and get(). Compare this to good number of controlled value extraction methods when it is used as return value (to name a few, orElse, orElseGet, map, flatMap, and even more to come in Java 9).
  3. java.util.Optional is not Serializable, which puts an end to call your method via remote interface (this, of course, implies that you care about calling it via remote interface).

Comments

-1

Does this code work for you?

for(Map.Entry<String, String> entry : map.entrySet()) {
     if(entry.getKey().equals(id)) {
         if (value.isPresent() && entry.getValue().equals(value.get())) {
             System.out.println(String.format("value present for id %s", id));
             return true;
         }
     }
}
return false;

I tried this one aswell, but return functions in lambda don't work :/

map.forEach((key, mapValue) -> {
        if(key.equals(id)) {
            if (value.isPresent() && mapValue.equals(value.get())) {
                System.out.println(String.format("value present for id %s", id));
                return true; //doesn't work -> Unexpected return value
            }
        }
    });

EDIT: I found a way to do it ^^

return map.entrySet().stream().anyMatch(entry -> entry.getKey().equals(id) && value.isPresent() && entry.getValue().equals(value.get()));

6 Comments

You iterate the whole map looking for values matching a predicate that may never return true if value.isPresent() == false.
Did you read his code? His method is even called "isValuePresent". It's just logical that he wants to test value.isPresent()... @MikhailProkhorov
sure. but he also has a Map with potentially O(1) complexity for key lookup, and Optional<Value>, where if there's no value in Option, he must logically immediately return false. Instead your solution makes O(n) operations, especially if there's no value.
You mean my solution iterates through the whole map, even if there is no value in Optional<Value>?
it iterates the whole map especially if there is no value in Optional, since anyMatch() by its contract short-circuits after first thing found, which wouldn't be the case if you check for it in predicate. And it iterates over the map instead of using far more efficient Map::get.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.