1
\$\begingroup\$

How could the following function (the for loop) be transformed to a lambda expression or refactored to be cleaner:

public static Collection<AccessRuleTemplate> getCaRules(String roleName, Collection<Integer> currentcas) {
    Collection<AccessRuleTemplate> result = new ArrayList<AccessRuleTemplate>();
    boolean allcafound = false;
    List<AccessRuleTemplate> carules = new ArrayList<AccessRuleTemplate>();
    for (Integer caId : currentcas) {
        if (caId.equals(Integer.valueOf(BasicAccessRuleSet.CA_ALL))) {
            allcafound = true;
            break;
        }
        carules.add(new AccessRuleTemplate(StandardRules.CAACCESS.resource() + caId.toString(), AccessRuleState.RULE_ACCEPT, false));
    }
    if (allcafound) {
        carules.clear();
        carules.add(new AccessRuleTemplate(StandardRules.CAACCESSBASE.resource(), AccessRuleState.RULE_ACCEPT, true));
    }
    result.addAll(carules);
    return result;

}

In particular i don't like the use of boolean flag and would like to avoid it. The idea with the function is to traverse list of ids and if a particular id found it breaks the loop and sets a flag to true and then checks the flag later and clears idlist and only adds something to list. Otherwise all other ids are added to list.

\$\endgroup\$
5
  • \$\begingroup\$ Welcome to Code Review. This looks like example code, which is off-topic for this site. Please edit your question to include your real, working code. \$\endgroup\$
    – Phrancis
    Sep 28, 2017 at 19:33
  • \$\begingroup\$ In addition, please tell us the task that this code accomplishes, and make that the title of the question. See How to Ask. \$\endgroup\$ Sep 28, 2017 at 19:44
  • \$\begingroup\$ Edited the question. \$\endgroup\$
    – Vombat
    Sep 28, 2017 at 20:44
  • \$\begingroup\$ You have fixed the "Not real code" problem, but you would still be Unclear what you're asking. What does this code do? What kind of input does it take and what kind of output should it return? \$\endgroup\$
    – mdfst13
    Sep 29, 2017 at 0:50
  • \$\begingroup\$ I would unlock this. His (edited) code looks clear enough, and he describes the problem. Looks OK to me. \$\endgroup\$
    – JimB
    Sep 29, 2017 at 7:01

1 Answer 1

3
\$\begingroup\$

There's a lot of waste in this method. You're copying things to idList, only to clear them. You're creating idList, only to copy it to result.

Try this:

public static Collection<AccessRuleTemplate> getCaRules(String roleName, Collection<Integer> currentcas) {
    List<AccessRuleTemplate> carules = new ArrayList<>();
    if (currentcas.contains(Integer.valueOf(BasicAccessRuleSet.CA_ALL))) {
        carules.add(new AccessRuleTemplate(StandardRules.CAACCESSBASE.resource(), AccessRuleState.RULE_ACCEPT, true));
    } else {
        for (Integer caId : currentcas) {
            carules.add(new AccessRuleTemplate(StandardRules.CAACCESS.resource() + caId.toString(), AccessRuleState.RULE_ACCEPT, false));
        }
    }
    return carules;
}

So, in the case where the item is found, there's a big savings - you don't create a bunch of objects and discard them. In the case where the item is not found, there's a very slight increase in time to traverse the list twice. In both cases, the copying between lists is eliminated, making this much more efficient (and readable).

I'd also recommend that for the "found" case, you pre-create a static, immutable, singleton list which you can directly return each time.

private static final Collection<AccessRuleTemplate> BASE_ACCESS_TEMPLATE
        = Collections.singletonList(new AccessRuleTemplate(StandardRules.CAACCESSBASE.resource(), AccessRuleState.RULE_ACCEPT, true));

Also "roleName" is unused. I'm guessing it was only used in logging which was stripped out.

Finally, though you could use a lambda function for the conversion loop, I think the code above is simpler. If you really want to, it would look something like:

return currentcas.stream()
        .map(caId -> {new AccessRuleTemplate(StandardRules.CAACCESS.resource() + caId.toString(), AccessRuleState.RULE_ACCEPT, false)})
        .collect(Collectors.toList());
\$\endgroup\$
1
  • \$\begingroup\$ You have to be careful using the static singleton collection, as you do not know if the resulting AccessRuleTemplate will be modified or not. If it can be modified, this will lead to interesting bugs :) \$\endgroup\$ Sep 29, 2017 at 8:06

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.