HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Dynamic condition on string in method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stringconditionmethoddynamic

Problem

I have the following method:

Float getPriceEqualsCategoryName(BrowseNode node, Float productPrice) {
    String name = node.getName();
    for (Entry> entry : categoryToFee.entrySet()) {
        if (name.equals(entry.getKey())) {
            List values = entry.getValue();
            for (RangeValue value : values) {
                if (productPrice < value.getMinValue())
                    return value.getMinValue();
                if (value.getRange().contains(productPrice))
                    return value.getReferPercentFee() / 100 * productPrice;
            }
        }
    }
    if (node.getAncestors() == null)
        return 0f;
    return getPriceEqualsCategoryName(node.getAncestors().getBrowseNode().get(0), productPrice);
}


In the above method I have to loop over something similar to tree structure bottom up and check if the name of the node EQUALS to entry name in the map. if yes - I will calculate the price according to it.

As a second choice, I need to loop again to check if the name CONTAINS the entry name in the map.

I wrote a similar method:

Float getPriceContainsCategoryName(BrowseNode node, Float productPrice) {
        String name = node.getName();
        for (Entry> entry : categoryToFee.entrySet()) {
            if (name.contains(entry.getKey())) {
                List values = entry.getValue();
                for (RangeValue value : values) {
                    if (productPrice < value.getMinValue())
                        return value.getMinValue();
                    if (value.getRange().contains(productPrice))
                        return value.getReferPercentFee() / 100 * productPrice;
                }
            }
        }
        if (node.getAncestors() == null)
            return 0f;
        return getPriceContainsCategoryName(node.getAncestors().getBrowseNode().get(0), productPrice);
    }


As you see, the only difference is the name.equals vs name.contains in the 4th line.

I don't like this duplicated

Solution

Extract Duplication

You could extract parts of the duplication to their own method:

private Float someResonableName(Float productPrice, List values) {
    for (RangeValue value : values) {
        if (productPrice < value.getMinValue()) {
            return value.getMinValue();
        }
        if (value.getRange().contains(productPrice)) {
            return value.getReferPercentFee() / 100 * productPrice;
        }
    }
    return -1;
}


And use it like this:

if (name.contains(entry.getKey())) {
            Float result = someResonableName(productPrice, entry.getValue());
            if (result != -1) {
                return result;
            }
        }


Use Interface

You could also use an interface:

interface NameKeyRelation {
    boolean isRelation(String name, RangeValue key);
}

Float getPriceContainsCategoryName(BrowseNode node, Float productPrice) {
    return getPriceContainsCategoryName(node, productPrice, (String name, RangeValue key) ->  name.contains(key)));
}

Float getPriceEqualsCategoryName(BrowseNode node, Float productPrice) {
    return getPriceContainsCategoryName(node, productPrice, (String name, RangeValue key) ->  name.equals(key)));
}

Float getPriceCategoryName(BrowseNode node, Float productPrice, NameKeyRelation relation) {
    String name = node.getName();
    for (Entry> entry : categoryToFee.entrySet()) {
        if (relation.isRelation(name, entry.getKey())) {
            Float result = someResonableName(productPrice, entry.getValue());
            if (result != -1) {
                return result;
            }
        }
    }
    if (node.getAncestors() == null)
        return 0f;
    return getPriceCategoryName(node.getAncestors().getBrowseNode().get(0), productPrice);
}


Restructure

It seems that your method currently does two things: It finds a category by name, and it calculate/finds some kind of price for a category. I would write two completely separate methods for each tasks, and then try to reduce duplication.

Code Snippets

private Float someResonableName(Float productPrice, List<RangeValue> values) {
    for (RangeValue value : values) {
        if (productPrice < value.getMinValue()) {
            return value.getMinValue();
        }
        if (value.getRange().contains(productPrice)) {
            return value.getReferPercentFee() / 100 * productPrice;
        }
    }
    return -1;
}
if (name.contains(entry.getKey())) {
            Float result = someResonableName(productPrice, entry.getValue());
            if (result != -1) {
                return result;
            }
        }
interface NameKeyRelation {
    boolean isRelation(String name, RangeValue key);
}

Float getPriceContainsCategoryName(BrowseNode node, Float productPrice) {
    return getPriceContainsCategoryName(node, productPrice, (String name, RangeValue key) ->  name.contains(key)));
}

Float getPriceEqualsCategoryName(BrowseNode node, Float productPrice) {
    return getPriceContainsCategoryName(node, productPrice, (String name, RangeValue key) ->  name.equals(key)));
}

Float getPriceCategoryName(BrowseNode node, Float productPrice, NameKeyRelation relation) {
    String name = node.getName();
    for (Entry<String, List<RangeValue>> entry : categoryToFee.entrySet()) {
        if (relation.isRelation(name, entry.getKey())) {
            Float result = someResonableName(productPrice, entry.getValue());
            if (result != -1) {
                return result;
            }
        }
    }
    if (node.getAncestors() == null)
        return 0f;
    return getPriceCategoryName(node.getAncestors().getBrowseNode().get(0), productPrice);
}

Context

StackExchange Code Review Q#80143, answer score: 4

Revisions (0)

No revisions yet.