4
\$\begingroup\$

My code does the following:

There are two Maps with properties. I need to check if properties with the same key and value already exist, and if they do, throw an exception.

    Map<String, Object> existingProperties = requires.get(requiresIndex).getProperties();
    for (Map.Entry<String, Object> property : properties.entrySet()) {
        if (existingProperties.containsKey(property.getKey())){
            if (existingProperties.get(property.getKey()) instanceof String && property.getValue() instanceof String ){
                String existingValue = (String) existingProperties.get(property.getKey());
                String newValue = (String) property.getValue();
                if (existingValue.equals(newValue)){
                    throw new Exception("Property " + property.getKey() + " is existsing with value " + newValue + "in requires " + requiresName);
                }
            }
        }
    }

We are working on Java 8. Is there a better/shorter way to write it?

\$\endgroup\$
3
  • \$\begingroup\$ Is map.containsValue useful for what you need to do? \$\endgroup\$
    – chatton
    Commented Jan 25, 2018 at 8:41
  • \$\begingroup\$ Please add some more code, e. g. where requires is defined. If possible, the whole method. It would be good to see all code that is referenced in the code snippet. \$\endgroup\$ Commented Jan 25, 2018 at 9:11
  • \$\begingroup\$ The code you added as an update feels to me as if you wanted us to do your work. It would be more encouraging if you had applied some of the suggestions to the updated code. But then, asking a follow-up question would be more appropriate. \$\endgroup\$ Commented Jan 29, 2018 at 0:06

4 Answers 4

3
\$\begingroup\$

OK, since it was requested in the comments, here goes

  1. You can replace the for loop with a java 8 stream() of the EntrySet() collection of the properties map.

  2. You can replace all the nested if statements with a filter() you can have your choice of separate filter()s for each if statement, or just concatenate all of them together with && operator.

  3. after the filter(), you need to tell the stream to end (aka Short-circuit) by finding the first item that matches the filter.

  4. The filter() operation returns an Optional since it is possible that no item satisfies the filter's predicate. in the question, it is required to take an action only if a match is found, so you can add an ifPresent() that takes a Consumer which does something to the matched item and returns void. ifPresent() does nothing if no item satisfies the filter's predicate.

Note: the posted code does not specify how newValue and requiresName which are mentioned in the throw statement are declared. note that these will have to be final in order to be included in a lambda expression.

properties.entrySet().stream()
    .filter(property -> 
        existingProperties.containsKey(property.getKey()) &&
        existingProperties.get(property.getKey()) instanceof String  && 
        property.getValue() instanceof String  &&
        existingProperties.get(property.getKey()).equals(property.getValue()))
    .findFirst()
    .ifPresent(property -> {
        throw new Exception("Property " + property.getKey() + " is existsing with value " + newValue
                + "in requires " + requiresName);
    });

The reasons why you would want to use the stream feature

  1. The actual iteration over the collection is left to the stream library which may decide to optimize this instead of sequential loop.
  2. The code is more concise and clear.
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Please do not just add a new version of the code, but also a review of the original code and an explaination of your code. \$\endgroup\$ Commented Jan 25, 2018 at 9:04
  • \$\begingroup\$ hmm... and here I was thinking that stream's fluent api is self explanatory... \$\endgroup\$ Commented Jan 25, 2018 at 9:05
  • 1
    \$\begingroup\$ Definitely not for beginners. And also please explain what is wrong with the original code. Otherwise we just have two versions of the same code, not a review. \$\endgroup\$ Commented Jan 25, 2018 at 9:08
  • 1
    \$\begingroup\$ Thanks for the answer, can you please explain in the lines (code) how it's better from the existing implementation ? stream/filter , why it's better. thanks a lot! \$\endgroup\$ Commented Jan 25, 2018 at 9:20
  • \$\begingroup\$ added benefits of stream feature \$\endgroup\$ Commented Jan 25, 2018 at 9:29
6
\$\begingroup\$

Sharon Ben Asher has a good answer using streams. I'd like to add some suggestions for the original code, since beginners are often not familiar with streams in Java.

You have some redundancy in your code, meaning code that is executed multiple times, which can hurt readability, decrease performance and introduce bugs. To tackle the issue, you can save results from method calls in variables and just access the variables multiple times. This also allows you to give the result a new name, and although it might make the code slightly longer in terms of lines of code, it can also make it a lot cleaner, and the individual lines shorter.

The most important example is calling property.getKey() and property.getValue() multiple times. Instead, do that at the beginning of the loop, and save the results in variables.

for (Map.Entry<String, Object> property : properties.entrySet()) {
    String key = property.getKey();
    Object value = property.getValue();

Then you access them in your code, which makes it easier to see where the same values are used. You are using the same method call to access another element of a different map, so again you can use a variable to save the result and make the code clearer and shorter:

    if (existingProperties.containsKey(key)) {
        Object existingObject = existingProperties.get(key);
        if (existingObject instanceof String && value instanceof String) {
            String existingValue = (String) existingObject;
            String newValue = (String) value;

We can improve readability by formatting very long lines (like that long string concatenation) on 2 or 3 lines, introducing newlines where we can devide it into logical parts:

            if (existingValue.equals(newValue)) {
                throw new Exception("Property " + key +
                                    " is existing with value " + newValue +
                                    " in requires " + requiresName);
            }

By the way, there seems to have been a space missing at the start of the string "in requires ", and a typo in " is existsing with value ".

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

Your current code looks like a big block of black characters. I find it difficult to read, since there are so many words in it. Therefore, my first idea was to paste this code into the IDE and let the IDE transform it into something more readable.

static void checkForDuplicates() {
    Map<String, Object> existingProperties = requires.get(requiresIndex).getProperties();
    for (Map.Entry<String, Object> property : properties.entrySet()) {
        if (existingProperties.containsKey(property.getKey())) {
            if (existingProperties.get(property.getKey()) instanceof String && property.getValue() instanceof String) {
                String existingValue = (String) existingProperties.get(property.getKey());
                String newValue = (String) property.getValue();
                if (existingValue.equals(newValue)) {
                    throw new Exception("Property " + property.getKey() + " is existsing with value " + newValue + "in requires " + requiresName);
                }
            }
        }
    }
}

I first wrapped the piece of code in a properly named method. This is something you should have done before posting the code here, to make clear what the input is and where it comes from.

Then, I formatted the code to get rid of the inconsistent spacing. Watching for the basic layout of the code is not a humane task, therefore the IDE should do it for you.

Now, the code doesn't compile. In a strict sense, this could already make your question off-topic for this site, which requires working code. And code that doesn't even compile does not work by definition.

After converting all unknown variables into parameters, the code is:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) throws Exception {

    for (Map.Entry<String, Object> property : properties.entrySet()) {
        if (existingProperties.containsKey(property.getKey())) {
            if (existingProperties.get(property.getKey()) instanceof String && property.getValue() instanceof String) {
                String existingValue = (String) existingProperties.get(property.getKey());
                String newValue = (String) property.getValue();
                if (existingValue.equals(newValue)) {
                    throw new Exception("Property " + property.getKey() + " is existsing with value " + newValue + "in requires " + requiresName);
                }
            }
        }
    }
}

Still no improvement on the readability. Let's ask the IDE whether it can make this code more beautiful. Iterating over a map is known to require a lot of code in Java before Java 8. But first, let's make the code a little lighter by extracting key and value to variables:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) throws Exception {

    for (Map.Entry<String, Object> property : properties.entrySet()) {
        String key = property.getKey();
        Object value = property.getValue();

        if (existingProperties.get(key) instanceof String && value instanceof String) {
            String existingValue = (String) existingProperties.get(key);
            String newValue = (String) value;
            if (existingValue.equals(newValue)) {
                throw new Exception("Property " + key + " is existsing with value " + newValue + "in requires " + requiresName);
            }
        }
    }
}

It's getting better. The black code slowly disappears, making place for more colors.

The next thing is to replace the throw new Exception with throw new IllegalStateException, since the latter does not have to be explicitly declared in the method definition (the throws Exception) and enables a nice refactoring in the next step.

And now the IDE (in my case IntelliJ) can automatically transform this large code into equivalent shorter code. To do that, place the cursor on the for keyword, press Alt+Enter and select Replace with Map.forEach:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) {

    properties.forEach((key, value) -> {
        if (existingProperties.get(key) instanceof String && value instanceof String) {
            String existingValue = (String) existingProperties.get(key);
            String newValue = (String) value;
            if (existingValue.equals(newValue)) {
                throw new IllegalStateException("Property " + key + " is existsing with value " + newValue + "in requires " + requiresName);
            }
        }
    });
}

This already helped a lot. Instead of two paragraphs of code, there is only one paragraph left.

In the next step, extract the call to existingProperties.get(key) into a variable (Ctrl+Alt+V), calling it existingValue. This produces a compile error because there is already another variable of the same name. We don't really need the two variables inside the if clause, so just remove them and replace them with the outer variables:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) {

    properties.forEach((key, value) -> {
        Object existingValue = existingProperties.get(key);
        if (existingValue instanceof String && value instanceof String) {
            if (existingValue.equals(value)) {
                throw new IllegalStateException("Property " + key + " is existsing with value " + value + "in requires " + requiresName);
            }
        }
    });
}

The text of the exception contains is missing a space before the in requires, but that is not easy to see. To make this code more readable, let the IDE replace the + operators with String.format by placing the cursor somewhere in the string and pressing Alt+Enter. From the menu, select Replace '+' with 'String.format':

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) {

    properties.forEach((key, value) -> {
        Object existingValue = existingProperties.get(key);
        if (existingValue instanceof String && value instanceof String) {
            if (existingValue.equals(value)) {
                throw new IllegalStateException(String.format("Property %s is existsing with value %sin requires %s", key, value, requiresName));
            }
        }
    });
}

The line containing the exception is still very long, so extract the exception message into a variable (Ctrl+Alt+V) and format it nicely. Then, replace the if conditions with a shorter, equivalent one:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) {

    properties.forEach((key, value) -> {
        Object existingValue = existingProperties.get(key);
        if (existingValue instanceof String && Objects.equals(existingValue, value)) {
            String message = String.format(
                    "Property %s is existsing with value %sin requires %s",
                    key, value, requiresName);
            throw new IllegalStateException(message);
        }
    });
}

The final step is fixing the typos (existing and in) in the exception message. Then you're done:

static void checkForDuplicates(
        Map<String, Object> existingProperties,
        Map<String, Object> properties,
        String requiresName) {

    properties.forEach((key, value) -> {
        Object existingValue = existingProperties.get(key);
        if (existingValue instanceof String && Objects.equals(existingValue, value)) {
            String message = String.format(
                    "Property %s is existing with value %s in requires %s",
                    key, value, requiresName);
            throw new IllegalStateException(message);
        }
    });
}
\$\endgroup\$
0
2
\$\begingroup\$

Understanding String.equals(Object)

Since String.equals(Object) cannot be overridden, it is safe to just test that the entries' value is a String. In other words, you only need one instanceof String check.

Iterating the smaller loop

Now, now, this smells a bit of premature optimization, but if you are certain that one of the Map is significantly smaller than the other and is likely to stay that way, you can consider iterating on the smaller loop.

Map.remove(Object, Object)

Streams are a little harder to understand, but I don't think that means the effort shouldn't be put in ;). There is a slightly shorter, and therefore maybe easier to understand, solution that relies on Java 8's new Map.remove(Object, Object) method to do the entry value comparison for us too.

Map<String, Object> copy = new HashMap<>(requires.get(requiresIndex).getProperties());
Optional<Map.Entry<String, Object>> duplicate = properties.entrySet().stream()
        .filter(property -> property.getValue() instanceof String
                                && copy.remove(property.getKey(), property.getValue()))
        .findFirst();
if (duplicate.isPresent()) {
    Map.Entry<String, Object> property = duplicate.get();
    throw new Exception("Property " + property.getKey() + " exists with value "
            + property.getValue() + " in requires " + requiresName);
}

In this approach, the second filtering condition checks if the copy of the 'requires map' contains the same entry that will be removed from the entry in properties. The first filtering condition is required to ensure at least one of the value is a String, per your question.

Still, this assumes that all the entries' values will strictly adhere to the general contract of Object.equals(Object), i.e. there is not going to be a case of MyType.equals(String) == true (since String.equals(MyType) == false).

You should also consider using a more appropriately-typed Exception class instead of the checked Exception. Over here, lambdas do no deal with checked exceptions, so I have used RuntimeException as an example.

edit

Since you want to also log all duplicate keys before running the value checks, you can insert a Stream.peek(Consumer) step in between your (now two) filter steps:

Optional<Map.Entry<String, Object>> duplicate = properties.entrySet().stream()
        .filter(property -> property.getValue() instanceof String
                                && copy.containsKey(property.getKey()))
        .peek(property -> LOG.warn("Property with key {} already exists in requires {}", 
                                property.getKey(), requires.get(requiresIndex).getName()))
        .filter(property -> copy.remove(property.getKey(), property.getValue()))
        .findFirst();
\$\endgroup\$
12
  • \$\begingroup\$ Thanks I'v try it and I got error unhanded exception, since it inside the stream, any idea ? \$\endgroup\$ Commented Jan 28, 2018 at 9:10
  • \$\begingroup\$ Streams do not like checked exceptions like Exception, I've updated my answer a little bit to clarify that point. \$\endgroup\$
    – h.j.k.
    Commented Jan 28, 2018 at 9:22
  • \$\begingroup\$ thanks but can you provide example in the code how I should handle the exception ? thanks! \$\endgroup\$ Commented Jan 28, 2018 at 9:24
  • \$\begingroup\$ Updated, and it's actually the lambda part not so much the stream processing. \$\endgroup\$
    – h.j.k.
    Commented Jan 28, 2018 at 9:25
  • \$\begingroup\$ You just change it to RuntimeException or something in addition ? \$\endgroup\$ Commented Jan 28, 2018 at 9:26

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.