How to write nested for loops in methodes (clean code)

Question

Clean code means for me: only one task for each methode and no nested loops.

When I got the following code, I asked myself, how can I avoid nested for loops and encapsulate them in methods.

private String getUser(){
    for (FieldConfigScheme context : getConfigurationSchemes()) {
        for (Option option : getOptions(context)) {
            for (Group group : getGroups()) {
                if (option.getValue().equalsIgnoreCase(group.getName())) {
                    return group.getUser();
                }
            }
        }
    }
    return "default";
}

My first solution was the following. The problem here is, the for loops are running until the end and do not break (return) when the value is found and set.

private String user = "default";

private String getUser(){
    for (FieldConfigScheme context : getConfigurationSchemes()) {
        processOptions(context);
    }
    return this.user;
}

private void processOptions(FieldConfigScheme context){
    for (Option option : getOptions(context)) {
        processGroups(option);
    }
}

private void processGroups(Option option){
    for (Group group : getGroups()) {
        setUser(option, group);
    }
}

private void setUser(Option option, Group group){
    if (option.getValue().equalsIgnoreCase(group.getName())) {
        this.user = group.getUser();
    }
}

so I wrote this code, which should be the same like the first:

private String user = "default";
private boolean isUserSet = false;

private String getUser(){
    for (FieldConfigScheme context : getConfigurationSchemes()) {
       if(!isUserSet) processOptions(context);
       else return this.user;
    }
    return this.user;
}

private void processOptions(FieldConfigScheme context){
    for (Option option : getOptions(context)) {
        if(!isUserSet) processGroups(option);
        else return;
    }
}

private void processGroups(Option option){
    for (Group group : getGroups()) {
        if(!isUserSet) setUser(option, group);
        else return;
    }
}

private void setUser(Option option, Group group){
    if (option.getValue().equalsIgnoreCase(group.getName())) {
        this.user = group.getUser();
        isUserSet = true;
    }
}

But then I asked myself, is this really better code? Is this more clean code? Yes, every method is only doing one thing. And yes, the code is better to read in my opinion. But from originally 12 lines compact code I now got 30 lines of code and one member variable more in the code. So is the first originally code better because it's more compact even with nested for loops?

What do you think? Which one is better? Or how can I write the code better?

Thanks in advance for your answers!

If you're using Java 8, you may find researching lambdas useful to your purpose. — Mena, Nov 09 '16 at 15:11
As @Mena mentioned, perhaps Java 8's lambda expressions are one option, but other than this I find your very first version to be the most preferable. You have only one method, and all the logic is contained in just a few nested loops. — Tim Biegeleisen, Nov 09 '16 at 15:11
*"Clean code means for me: only one task for each methode and no nested loops."* For me *Clean Code* means *readable, duplication free code*. So I also prefer the "nested loop" version. The reason for me is, that there is no other work within the individual loops. — Timothy Truckle, Nov 09 '16 at 15:18

score 0 · Accepted Answer · answered Nov 10 '16 at 23:04

Instead of returning void, why not boolean?

private String getUser(){
    for (FieldConfigScheme context : getConfigurationSchemes()) {
        if (processOptions(context)) {
            break;
        }
    }
    return this.user;
}

private boolean processOptions(FieldConfigScheme context){
    for (Option option : getOptions(context)) {
        if (processGroups(option)) {
            return true;
        }
    }
    return false;
}

private boolean processGroups(Option option){
    for (Group group : getGroups()) {
        if (option.getValue().equalsIgnoreCase(group.getName())) {
            this.user = group.getUser();
            return true;
        }
    }
    return false;
}

T.B.H. I prefer the nested loops method. It looks clean, there is nothing more going on in the loop than to simply find something in a hierarchy and this is perfectly fine.

The use of extra function in this case is just bad. Imagine having to debug this code now, rather than focusing on one method which is doing this, you will have to look at all the extra ones you made.

Also this method doesn't seem to take any parameters which suggests that it actually only needs to do this check once and the rest of the time it should just return the same value. That just a guess, but if that was the case, then it makes your efforts to make it cleaner all the more unnecessary.

How to write nested for loops in methodes (clean code)

1 Answers1