5
\$\begingroup\$

I wrote code which sorts items, based on parameters (IconConfiguration). I have a map of items which have index and item so basically a predefined list on which index item should be, and maxPositions parameter which is kind of a limit of how big my list can be. I wrote a code but its repetitive and pretty ugly when i check it, but i am unable to get idea on what to refactor.

private List<GWBaseData> applyItemsIndex(List<GWBaseData> gwInputBaseDataList) { //TODO: Refactor repetitive code?
        List<GWBaseData> gwBaseDataList = new ArrayList<>();

        if (iconConfiguration.getMax_positions() != -1) {
            for (int i = 0; i < gwInputBaseDataList.size(); i++) {
                if (gwBaseDataList.size() < iconConfiguration.getMax_positions()) {
                    if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
                        gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
                    } else {
                        for (int j = 0; j < gwInputBaseDataList.size(); j++) {
                            if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                                gwBaseDataList.add(gwInputBaseDataList.get(j));
                                break;
                            }
                        }
                    }
                }
            }
        } else {
            for (int i = 0; i < gwInputBaseDataList.size(); i++) {
                if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
                    gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
                } else {
                    for (int j = 0; j < gwInputBaseDataList.size(); j++) {
                        if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                            gwBaseDataList.add(gwInputBaseDataList.get(j));
                            break;
                        }
                    }
                }
            }
        }

        return gwBaseDataList;
    }

So basically the whole difference between both block is only one additional if (if (gwBaseDataList.size() < iconConfiguration.getMax_positions()))

Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

Add an extra local variable before the first loop:

int maxItems = iconConfiguration.getMax_positions();
if (maxItems == -1) {
    maxItems = gwInputBaseDataList.size();
}

This way if you didn't specify a max in the configuration you just use "all" input items.

You can go a little bit further as well and use this as the condition on the outer for loop:

for (int i = 0; i < maxItems; i++) {
    if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
        gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
    } else {
        for (int j = 0; j < gwInputBaseDataList.size(); j++) {
            if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
                gwBaseDataList.add(gwInputBaseDataList.get(j));
                break;
            }
        }
    }
}
return gwBaseDataList;

I couldn't actually test this code, so make sure it's not off by 1 on the index when you try it yourself. And just to be safe test if this still actually does what you want it to do for all edge cases.

EDIT bugfix:

In case the max from the configuration is larger than the actual size you'll get an index out of bounds. Fixed by changing the if condition to the folowing:

if(maxItems == -1 || maxItems > gwInputBaseDataList.size()){
Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ What happens if maxItems > input data list size? Could you minimize needless iterations by starting with gwInputBaseDataList.size(), and reducing that to iconConfiguration.getMax_positions() if necessary. \$\endgroup\$
    – jmarkmurphy
    Apr 3, 2018 at 13:24
  • \$\begingroup\$ That is indeed a bug in my code, that wasn't there in the original. Well spotted. \$\endgroup\$
    – Imus
    Apr 3, 2018 at 13:50
3
\$\begingroup\$

  • If you replace this condition:

    if (gwBaseDataList.size() < iconConfiguration.getMax_positions())

    With this condition:

    if (iconConfiguration.getMax_positions() == -1
        || gwBaseDataList.size() < iconConfiguration.getMax_positions())

    Then you can get rid of this outer construct:

    if (iconConfiguration.getMax_positions() != -1) {
    //...
} else {
    //...
}

    And you can delete everything that was enlosed in the else block of this construct. If I am not mistaken, this should do it.

  • Another suggestion: You can replace this:

    for (int j = 0; j < gwInputBaseDataList.size(); j++) {
    if (!gwBaseDataList.contains(gwInputBaseDataList.get(j))) {
        gwBaseDataList.add(gwInputBaseDataList.get(j));
        break;
    }
}

    With an enhanced for-loop:

    for (GWBaseData gwInputBaseData : gwInputBaseDataList) {
    if (!gwBaseDataList.contains(gwInputBaseData)) {
        gwBaseDataList.add(gwInputBaseData);
        break;
    }
}

    I find that easier to read, since, in this case, the index itself is irrelevant and only the item matters.

  • Refactoring further: Since there is no point in continuing the loop once gwBaseDataList is larger than the maximum number of items, you might as well specify this requirement in the termination condition of the for loop:

    for (int i = 0;
        i < gwInputBaseDataList.size()
        && (iconConfiguration.getMax_positions() == -1
        || gwBaseDataList.size() < iconConfiguration.getMax_positions());
        i++) {
    if (getItemsMap().get(i) != null && getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)) != null) {
        gwBaseDataList.add(i, getBaseDataInListById(gwInputBaseDataList, getItemsMap().get(i)));
    } else {
        for (GWBaseData gwInputBaseData : gwInputBaseDataList) {
            if (!gwBaseDataList.contains(gwInputBaseData)) {
                gwBaseDataList.add(gwInputBaseData);
                break;
            }
        }
    }
}

    That way, you have one nested if-block less.

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ +1 for enhanced for loop for the inner loop. The ending condition on the outer for loop is starting to get a bit too convoluted though. If mine works it's a lot easier to read, but I didn't actually test it :) \$\endgroup\$
    – Imus
    Apr 3, 2018 at 12:26
  • \$\begingroup\$ @Imus True, the termination condition is a bit crammed, although program-logic-wise, it is still more efficient than putting it into a separate if block because it will prevent redundant loop iterations. In fact, your solution seems to be more or less equivalent to the last code sample in my answer, with the exception that you store the effective maximum number of items in a temporary variable, which admittedly makes the code much easier to read and probably also more efficient, because then, the termination condition only needs to compare one pair of values instead of three. \$\endgroup\$
    – Stingy
    Apr 3, 2018 at 13:06
  • \$\begingroup\$ I expect the ending condition for a loop to just check things that are changeable within the loop. If the condition is the same for all iterations of the loop, then it is more readable IMHO to put it outside the loop. \$\endgroup\$
    – jmarkmurphy
    Apr 3, 2018 at 13:07
  • 1
    \$\begingroup\$ @Imus But putting the check outside the loop is exactly what caused the code duplication the OP was unhappy with in the first place. But putting it inside the loop would cause unnecessary loop iterations. So it has to go into the termination condition. And since, as you noted, my code checks the same condition before every iteration, it is better to create a temporary variable, as you did. \$\endgroup\$
    – Stingy
    Apr 3, 2018 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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