Looping over a map requiring casting and instance checks

Question

The following method works. I am looking for advice to make it more readable. To see if I could avoid any unneccesary checks / casting and the instance of check. Also not a fan of having to recreate the initial String with every loop but not sure if there is a way around it.

Things to note.
I am using Java 8 and Spring boot.
I can't change the productMap. It will remain as a Map < String, Object> .
Can use external library Apache commons but not Guava.
It is possible for initialText and productMap to be null / empty.
In the map, I am only looking for values which are of type String and value contains only numbers. If it exists, I want to use it and replace initialText

Example scenario with following data in the parameters being passed in:

String initialText = "This is a sample text with PLACEHOLDER and yet another PLACEHOLDER and more message."
productMap = {
    "Some key" : new SomeObject(), // I won't use this cos value is not String. 
    "PLACEHOLDER" : "26000", // I will use this to replace cos the key PLACEHOLDER exists in above text and it contains only numbers. 
    "SOME_OTHER" : "SOME_TEXT", // it is a String value but the key doesn't exist in the initial text thus no difference. 
}

After executing the following code, I expect the method to return the updated String as follows (notice the comma due to String format):

"This is a sample text with 26,000 and yet another 26,000 and more message."

Method as follows. Appreciate the advices. Thanks.

public String update(String initialText, Map<String, Object> productMap){
    if(StringUtils.isBlank(initialText) || MapUtils.isEmpty(productMap)){
        return initialText;
    }
    for (Map.Entry<String, Object> productData : productMap.entrySet()) {
        String key = productData.getKey();
        Object value = productData.getValue();
        if(value instanceof String && ((String) value).matches("[0-9]+")){
            String offerValue = (String)value;
            initialText = initialText.replaceAll(key, String.format("%,d", Integer.parseInt(offerValue)));
        }
    }
    return initialText;
}

Answer 1

public String update(String initialText, Map<String, Object> productMap){         //  1
    if(StringUtils.isBlank(initialText) || MapUtils.isEmpty(productMap)){         //  2
        return initialText;                                                       //  3
    }                                                                             //  4
    for (Map.Entry<String, Object> productData : productMap.entrySet()) {         //  5
        String key = productData.getKey();                                        //  6
        Object value = productData.getValue();                                    //  7
        if(value instanceof String && ((String) value).matches("[0-9]+")){        //  8
            String offerValue = (String)value;                                    //  9
            initialText = initialText.replaceAll(key, String.format("%,d", Integer.parseInt(offerValue)));   //10
        }                                                                         // 11
    }                                                                             // 12
    return initialText;                                                           // 13
}                                                                                 // 14

---

Line 1

The method and parameter names could be clearer. A method named update could do many things. How about something akin to replacePlaceholders. Analogously, initialText would become template. Also, the Hungarian notation for the map parameter isn't needed (How should I name a java.util.Map?). Maybe substitutionByPlaceholder or something similar. I feel like there could be a better name, but I lack the domain knowledge.

Also, I always make parameters, variables and such final. I swear by it, but maybe your style guide disallows it.

Making the parameters final also prevents you from reassigning them, which is usually a good thing, because it prevents silly mistakes like

public void setFoo(int foo) {
    foo = foo;
}

It looks obvious now, but in a more complex method, it can cause a bug real' quick.

Lines 5, 6, 7

Again, final variables are recommended.

Lines 6, 7

key and value aren't great names. How about placeholder and replacement?

Line 8

Your method of testing if the String is an int is flawed. How about negative Integers, or something like 100000000000000000000000000. That sure isn't an Integer. Be sure to think about corner cases like this!

If it's almost always an Integer or performance doesn't matter to you, you can just use Integer.parseInt wrapped in a try catch block. Deliberately catching Exceptions for control flow is usually not recommended though. For alternatives, see What's the best way to check if a String represents an integer in Java?

---

The rest looks okay, but it's a big method. Not huge by any means, but it still does too much. To quote Robert C. Martin in Clean Code:

Functions should do one thing. They should do it well. They should do it only.

If you look at your method, it does multiple things. It guards against bad input, it loops over the map, it checks if entries are valid, and it does the replacement.

If we split your method into four method, they all become just a few lines each, each method doing exactly one thing (Barring the last one, which does two things, checking if the String is an integer and replacing, which is a symptom of us using try-catch for control flow) that is very easy to understand.

Also all methods may be static since they don't use any instance members.

public static String replacePlaceholders(final String template, final Map<String, Object> productMap) {
    if (StringUtils.isBlank(template) || MapUtils.isEmpty(productMap)) {
        return template;
    }

    return replaceAllPlaceholders(template, productMap);
}

private static String replaceAllPlaceholders(final String template, final Map<String, Object> productMap) {
    String result = template;

    for (final Map.Entry<String, Object> productData : productMap.entrySet()) {
        result = replacePlaceholderIfValueIsIntegerString(result, productData);
    }

    return result;
}

private static String replacePlaceholderIfValueIsIntegerString(final String template, final Map.Entry<String, Object> entry) {
    if (entry.getValue() instanceof String) {
        return replacePlaceholderIfReplacementIsInteger(entry.getKey(), (String) entry.getValue(), template);
    }
    return template;
}

private static String replacePlaceholderIfReplacementIsInteger(final String placeholder, final String replacement, final String template) {
    try {
        return template.replaceAll(placeholder, String.format("%,d", Integer.parseInt(replacement)));
    } catch (final NumberFormatException e) {
        return template;
    }
}

Recommended reading:

• Robert C. Martin; Clean Code

• Joshua Block; Effective Java, 3rd Edition

Answer 2

Single Responsibility

Your function do 2 things:

1. Filter the map to contain only numbers
2. Fill template string with relevant values

I suggest to separate to 2 functions.

Filter the map

Will be more readable with Java Streams filter and map.

The result should be a map from string to int.

Fill template

Can be done with Java streams reduce or collect.

StringBuilder could be usufull here but it doesn't contains a function replaceAll.

Code review

• Don't reassign input parameters. It will make it harder for you to debug and it is confusing because the name don't fit the purpose of the variable. Do this instead

  string res =initialText and keep changing res.

• I like to name maps in following format: keyName2valueName. In your case placeholder2number.

• The following code can be removed and the function still works.

if(StringUtils.isBlank(initialText) || MapUtils.isEmpty(productMap)){
        return initialText;
            }

I don't think it helps very much to the performance either.