current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I have the following method:

Float getPriceEqualsCategoryName(BrowseNode node, Float productPrice) {
    String name = node.getName();
    for (Entry<String, List<RangeValue>> entry : categoryToFee.entrySet()) {
        if (name.equals(entry.getKey())) {
            List<RangeValue> 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<String, List<RangeValue>> entry : categoryToFee.entrySet()) {
            if (name.contains(entry.getKey())) {
                List<RangeValue> 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 code.. is there a better way to write it?

share|improve this question

3 Answers 3

up vote 2 down vote accepted

Step 1

Going on a hunch here, but may I suppose that you have something similar to the following to determine which method to call?

if (exactMatch) {
    getPriceEqualsCategoryName(node, productPrice);
} else {
    getPriceContainsCategoryName(node, producePrice);
}

If that is the case, and without delving too much into further refactoring the logic for this step, it may be more convenient, or easier, to pass exactMatch as an argument to a generic method:

float getPriceForMatchingCategoryName(final BrowseNode node, float productPrice, 
        boolean exactMatch) {
    final String name = node.getName();
    for (final Entry<String, List<RangeValue>> entry : categoryToFee.entrySet()) {
        final String key = entry.getKey();
        if (name.contains(key) && (!exactMatch || name.equals(key))) {
            // ...
        }
    }
    // ...
}

(I have taken the liberty to change Float to float to eliminate some auto-boxing.)

Using interfaces or lambdas is still ok, but I can't help but feel that these will be overblown solutions for a small piece of boolean logic. As illustrated, exactMatch is used as a secondary condition after name.contains(key) since contains() is the encompassing condition for both. If an exact match is not required, the primary condition will satisfy, else we will also perform name.equals(key).

The downside is that two comparisons will be done for equals(), so if you have some really strict performance ceiling and this is indeed one of the last bottlenecks to eliminate, then you should just use a simpler if statement to switch between the comparisons to perform.

Step 2

The next refactoring step that you may want to consider is to abstract the double-if pricing logic somewhere else. The other answers suggested putting them into a method, but can they be regarded as to sufficiently 'belong' to the RangeValue class itself? If so, the body of the if clause above can be simplified as such:

// Using Java 8 interfaces with default methods to do my own 'stubbing'
private static interface RangeValue {
    float getMinValue();

    Collection<Float> getRange();

    float getReferPercentFee();

    default boolean isEligiblePrice(float price) {
        return price < getMinValue() || getRange().contains(Float.valueOf(price));
    }

    default float getPrice(float price) {
        return price < getMinValue() ? getMinValue() : getReferPercentFee() / 100 * price;
    }
}

////

if (name.contains(key) && (!exactMatch || name.equals(key))) {
    for (final RangeValue current : entry.getValue()) {
        if (current.isEligiblePrice(productPrice)) {
            return current.getPrice(productPrice);
        }
    }
}

Step 3

The final, minor refactoring you can do is to simplify your last return statement:

float getPriceForMatchingCategoryName(final BrowseNode node, float productPrice,
        boolean exactMatch) {
    final String name = node.getName();
    for (final Entry<String, List<RangeValue>> entry : categoryToFee.entrySet()) {
        final String key = entry.getKey();
        if (name.contains(key) && (!exactMatch || name.equals(key))) {
            for (final RangeValue current : entry.getValue()) {
                if (current.isEligiblePrice(productPrice)) {
                    return current.getPrice(productPrice);
                }
            }
        }
    }
    return node.getAncestors() == null ? 0f : getPriceForMatchingCategoryName(
        node.getAncestors().getBrowseNode().get(0), productPrice, exactMatch);
}
share|improve this answer
    
I liked the first refactoring. However, I didn't understand the RangeValue approach - how do I implement it? it's a class today not an interface –  Dejel Feb 10 at 18:33
    
You just need to include the two methods proposed. I had to 'stub' RangeValue as an interface in my own testing since I do not know the original code to that class. ;) –  h.j.k. Feb 11 at 0:46
up vote 3 down vote

You can extract everything you do in the if-condition to another method and reuse that method in both cases. I.e. extract this bit and pass entry to it as a parameter and return a value. 

            List<RangeValue> values = entry.getValue();
            for (RangeValue value : values) {
                if (productPrice < value.getMinValue())
                    return value.getMinValue();
                if (value.getRange().contains(productPrice))
                    return value.getReferPercentFee() / 100 * productPrice;
            }

Alternatively you can pass a boolean flag exactMatch to your existing method and either check for equals or contains based on that flag.

share|improve this answer
up vote 4 down vote

Extract Duplication

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

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;
}

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<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);
}

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.

share|improve this answer
    
how can I use the interface? –  Dejel Feb 10 at 11:29
    
@Odelya like in my example code (which is more psydo-code and might have typos, etc). I used Java8 lambda syntax, but you could also use the old fashioned way with new NameKeyRelation() { public boolean isRelation(String name, RangeValue key) {return name.equals(key);}} –  tim Feb 10 at 11:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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