Nesting versus GOTO: which is better to avoid?

Question

In Java they're not really known as GOTO statements and are rather referred to as Branching Statements, but I find that the former term is a bit more indicative of what they actually do.

Regardless, I've been designing some code recently which has me in a bind. On the one hand, I've been taught to avoid avid use of such statements because they make code both non-linear and more difficult to follow. They of course have excellent uses, and I'm not one of those puritans who says that they should be discarded at any cost.

On the other hand, there's also a common tenet that says I ought to avoid deep nesting of conditions and loops if it's possible and reasonable to do so.

So here's my conundrum. I'm hoping someone can guide me and give me some solid reasoning and best practices advice, because apparently my brain can't handle the conflicting signals.

Several times through my code, I have a for() loop which is something like the following (where lines is a List<String> and values is a Map<String, String>):

for(String line : lines) {

    if(line.charAt(0) == '#') {
        LOGGER.debug("Skipping commented line in file.");
        continue;
    }

    line = line.trim().toLowerCase();
    String[] pair = line.split("=");

    if(pair.length != 2) {
        LOGGER.error("Skipping malformed line in file: " + line);
        continue;
    }

    pair[0] = pair[0].trim();
    pair[1] = pair[1].trim();

    if(values.containsKey(pair[0])) {
        LOGGER.debug("Value is already assigned.");
        continue;
    }

    values.put(pair[0], pair[1]);

    //...and so on with still more processing
}

The reason I gave a little bit more in this code snippet than is strictly required to understand the use case is because I think the fact that the for loop isn't really "tight" and that it performs a lot of processing is relevant to the style choice. To me, it makes the code feel somewhat sloppy and messy to have multiple areas scattered throughout it which bump the reader back to the beginning of the loop. In other words, it feels a lot like the good old GOTO that everyone loves to hate on.

So my other design option is to write it with the following style:

for(String line : lines) {
    if(!line.charAt(0) == '#') {
        line = line.trim().toLowerCase();
        String[] pair = line.split("=");
        if(pair.length == 2) {
            pair[0] = pair[0].trim();
            pair[1] = pair[1].trim();
            if(!values.containsKey(pair[0])) {
                values.put(pair[0], pair[1]);
                //...and so on with still more processing
            }
            else {
                LOGGER.debug("Value is already assigned.");
            }
        }
        else {
            LOGGER.error("Skipping malformed line in file: " + line);
        }
    }
    else {
        LOGGER.debug("Skipping commented line in file.");
    }
}

Both of these techniques/styles makes my eye twitch spastically for various reasons, and I keep rewriting my code to fit whichever rationalization wins out in the current moment. I also think that if you definitively choose one as "The Style" to always go with, it's trivial to extend the above code snippets to the point where it makes more sense to go with the other.

Does anyone have a more definitive statement of when to use one style over the other? Or even just some good reasoning to apply to a situation to make sure you are following the best rule and coding practice? Any general tips, tricks, and advice for dealing with these sorts of situations?

Answer 1

A different way is to let these pairs be represented by a class, which itself has a static factory method that will return null on failure.

public final class Pair {
    private String key;
    private String value;

    public Pair(key, value) {
        // TODO: Add proper argument checking/throw exceptions.

        this.key = key;
        this.value = value;
    }

    public String getKey() {
        return key;
    }

    public String getValue() [
        return value;
    }

    /**
     * Returns the string representation which looks like @{code key=value}.
     */
    @Override
    public String toString() {
        return key + "=" + value;
    }
}

And the static factory method:

/**
 * Constructs a Pair from the given string,
 * returns null if the string is misformed or
 * the string was null.
 * 
 * A well formed input looks like {@code key=value} and
 * does not start with a {@code #} as that indicates comments.
 */
public static Pair fromString(String str) {
    // We do not accept nulls and empty strings.
    if (str == null || str.empty()) {
        return null;
    }

    // TODO: Figure out if trimming of str would be a good idea.

    // Skip comments.
    if (str.startsWith("#")) {
        return null;
    }

    String[] splitted = str.split("=");

    // We also do not accept anything else.
    if (splitted.length != 2) {
        return null;
    }

    // TODO: Are you accepting zero-length keys/values?

    return new Pair(splitted[0], splitted[1]);
}

It basically looks the same as your loop, with two important differences:

1. It's a method of its own, easily reusable, easily testable.
2. It has documentation what the input should look like.

Now to your loop:

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null && !values.containsKey(pair.getKey())) {
        // TODO
    }
}

That does not only shorten the code inside the loop, but will in the end make it more readable because you're now using pair.getValue() instead of pair[1].

Based on the context of this I would suggest other names, though, like StringPair, Setting, SettingsPair or ConfigEntry.

Here is your loop with the log statements (assuming that the LOGGER has an overload that works similar to Logger.log(...)):

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null) {
        if (!values.containsKey(pair.getKey()) {
            // TODO
        } else {
            LOGGER.debug("Pair is already assigned: {0}", pair);
        }
    } else {
        LOGGER.error("Skipping line: {0}", line);
    }
}

There are no fine-grained error messages there because it should be obvious from the input why the fromString function could not create the Pair.

Though we're back at two if-statemens with else branches, readability is a lot better.

---

This does violate Item 43 from Effective Java (2nd Edition) which goes like this:

Return empty arrays or collections, not nulls

But why did I than suggest this approach? Effective Java states that it is better to throw fine grained exceptions rather then return null to ease dealing with the method. This is true for any sort of method, but it is not what this approach is supposed to do. We don't care what kind of problem there is, we just want an object that is usable for our program logic or move on, discarding fine grained errors in the process.

If you use this approach you need to keep some things in mind:

• Don't use a constructor for this purpose but a static factory method.
• Document the behavior and purpose of the static factory method thoroughly.
• It should be obvious when seeing the input what the function returns.
• Know when to use this and when to use exception handling (be aware why this is not a generic all-in-one solution to everything).
• If this is inside a library/public API, always provide a way that does throw fine grained exceptions.

---

In case you're parsing ini or settings file with this which do not require "special" treatment, the answer from AJMansfield is correct.

Do not roll your own version if java.util.Properties works for it.

Answer 2

Your first excerpt is fine, and it is better than your second. Your second excerpt is worse because of the deeper nesting. (Also, if you were to choose nesting anyway, I'd invert the conditions to put the shorter code branch first to reduce mental workload when reading the code.)

Consider why the goto statement is considered harmful. Since it has the potential to jump to arbitrary places in the code, it could turn your program into a pile of spaghetti.

The use of continue is not a problem. You still have a structured loop, and the only thing it can do is proceed to the top of the loop to process the next element (if there is one). No chance of spaghetti there.

If you don't care about logging, you could use a capturing regular expression, which takes care of your first two conditions as well as trimming whitespace. You would only have to worry about whether the key already exists.

Answer 3

I find it strange that the zero-length line problem has not yet been pointed out:

for(String line : lines) {

     if(line.charAt(0) == '#') {
         ....

The code above assumes a non-empty line in all cases, otherwise you get IndexOutOfBoundsException.

This is the sort of issue that should be identified and covered with Unit testing.

This also leads on to the fact that you appear to be reinventing-the-wheel, and should consider the suggestion for using java.util.Properties.

Despite that suggestion, I have to weigh in on the continue/break theoretical discussion as well. Loop entry and exit points are already the targets of branch instructions in the compiled (JIT or Byte) code that Java produces. These branch points are well managed, and have clean, and well documented entry/exit conditions. They are nothing like the concept of a GOTO. I have, over time, become very comfortable with using them, and they are no more distrustful than an early-return from a method call. In fact, conceptually, that is what they are, simply an early-termination of a block of code that happens to be in a loop, and the termination may allow (continue) or disallow (break) further iteration.

I would not recommend using these statements with impunity, but, the fact that break is the natural syntax in the switch statement gives some hint that it's OK to use. It is the right tool for some jobs. Use it when appropriate.

Answer 4

Your problem is not with the loop construct - nor is it with GOTOs. Your problem is you are avoiding some tools that can make your code better.

Here's an example.

enum Validator {
    IsNotComment("Skipping commented line in file.") {

        @Override
        boolean isValid(String s) {
            return !s.startsWith("#");
        }

    },
    IsAPair("Not a pair") {

        @Override
        boolean isValid(String s) {
            return s.trim().split("=").length == 2;
        }

    };

    abstract boolean isValid(String s);

    public final String failMsg;

    Validator(String failMsg) {
        this.failMsg = failMsg;
    }
}

public void test() {
    String[] lines = {"Hello"};
    Map<String,String> values = new HashMap<>();
    for (String line : lines) {
        boolean valid = true;
        String failMsg = "";
        for ( Validator v : Validator.values() ) {
            valid &= v.isValid(line);
            if ( !valid ) {
                failMsg = v.failMsg;
            }
        }
        if ( valid ) {
            // Do stuff with valid lines.
            String [] parts = line.trim().split("=");
            values.add(parts[0],parts[1]);
        } else {
            System.out.println("Failed validation: "+failMsg);
        }
    }
}

See that by refactoring properly you can now add more validations steps without affecting the main code, each one will only add to the quality. You have now transformed brittle code that is comparably difficult to understand and enhance into something clear and flexible.

This encourages you to focus on the fact that you are trying to mix validation with parsing.

If you let your worries guide you properly you will see that the validation is a separate process and your attempt to mix the validation into the parsing process is making your code smell.

Validate first - then parse. Do not pre-optimise and merge them into one until you have a good measured reason to do so - and even then measure again.

Added

Just for the joy of it - here's what it might look like in Java 8

List<String> lines = Arrays.asList(
        "# Comment",
        "",
        "A=B",
        "A=C",
        "X=Y ",
        "P = Q");

public void test() {
    Map<String,String> values = new HashMap<>();
    lines.stream()
            // Trim it and lowercase.
            .map(s -> s.trim().toLowerCase())
            // Discard empty lines - good call @rolfl
            .filter(s -> !s.isEmpty())
            // Discard comments.
            .filter(s -> s.charAt(0) != '#')
            // Split the trimmed form
            .map(s -> s.split("="))
            // Must be two parts.
            .filter(a -> a.length == 2)
            // Trim each one.
            .map(a -> new String[]{a[0].trim(), a[1].trim()})
            // Not seen already.
            .filter(a -> !values.containsKey(a[0]))
            // Install in values.
            .forEach (a -> values.put(a[0], a[1]));
    System.out.println(values);
}

This really actually prints:

{p=q, a=b, x=y}

Now is that cool or what?

Sorry but it doesn't do all your helpful printing and stuff.

Answer 5

Here is a far and away superior solution: use java.util.Properties to handle your configuration file. Properties supports a key=value syntax that is very similar to the one you are trying to parse. It's up to you to decide whether that syntax is close enough to handle any existing configuration files you might have, and whether the validation and logging in your current code are essential.

Answer 6

I agree with @200_sucess that the first version is better.

An alternative may be a subroutine:

for(String line : lines) {
    AddLine(line, values);
    //...and so on with still more processing
}

void AddLine(String line, Map<String, String> values)
{
    if(line.charAt(0) == '#') {
        LOGGER.debug("Skipping commented line in file.");
        return;
    }

    line = line.trim().toLowerCase();
    String[] pair = line.split("=");

    if(pair.length != 2) {
        LOGGER.error("Skipping malformed line in file: " + line);
        return;
    }

    pair[0] = pair[0].trim();
    pair[1] = pair[1].trim();

    if(values.containsKey(pair[0])) {
        LOGGER.debug("Value is already assigned.");
        return;
    }

    values.put(pair[0], pair[1]);
}

Do you mind the "early return" in a subroutine? Would you goto the end of the subroutine in order to return, or build a deeply-nested if?

Answer 7

Here's an answer out of left-field, but it may give you some inspiration. If I were doing this in Haskell I would write it as a pipeline of maps and folds on a list. I don't know much about Java, but could you rewrite your function as several smaller functions each of which acts on an iterator and produces a new iterator to feed into the next function?

processLines = insertInto dict
              . map trimPairs
              . filter notMalformed
              . map splitLine
              . filter notComment

Answer 8

Let me echo @AJMasfield that for this particular example Don't Reinvent the Wheel. However, this question comes up in contexts other than Properties. In the second version, the LOGGER messages are further and further away from the relevant if clauses. The first version uses an idiom we should be familiar with, checking preconditions and error exits up at the top. I find it quite readable.

Answer 9

The solution to your problem (given in the example):

1. Select the contents of your for block
2. Refactor -> Extract method
3. Inspect program to make sure nothing broke
4. Inspect the new generated method to see if it's ugly
5. If ugly, select ugly parts, go to #2
6. If you can't select them because they are too scattered in the body, rearrange until you can
7. If you can't rearrange because code would break, reconsider your algorithm design - does it really have to be this convoluted?
8. If yes, accept it and move on. Not all code can be beautiful, and sometimes perfectionism isn't worth it.

---

The answer to your question (example aside):

goto is very rarely needed in modern languages. Part of the reason is that things like return, break, continue and throw exist. These are basically very limited versions of goto that are incapable of causing anywhere near as much chaos, but can still satisfy a popular goto use case. It is unlikely that you will encounter goto worthy code which cannot be tamed using these tools and applying some intelligent refactoring.

Answer 10

I agree with others that the first one is better because it's easier to read but @AJMansfield also has a really good point. It's worth mentioning that you can also use Guava's Splitter and MapSplitter classes or write something similar.

Answer 11

Base on the notion "ask forgiveness rather than permission", I would put the line handling in a try-catch block, like so :

for(String line : lines) {

    try {
        if(line.charAt(0) == '#') {
            throw new Exception("Skipping commented line in file.");
        }

        line = line.trim().toLowerCase();
        String[] pair = line.split("=");

        pair[0] = pair[0].trim();
        pair[1] = pair[1].trim();

        if(values.containsKey(pair[0])) {
            throw new Exception("Value is already assigned.");
        }

        values.put(pair[0], pair[1]);

        //...and so on with still more processing
    } catch (Exception e) {
        LOGGER.error("Skipping line in file: " + line + " " + e.getMessage());
    }
}

This can catch any other case you may not have considered, such as the line being null or empty, or any thing else in the line processing.

I would probably extract a method to include the values.put with the containsKey test that would throw the exception. Any kind of test you need to perform can be extracted to a method that throws an exception if the condition is not met, such as

public void testComment(String line) throws Exception {
    if (line.charAt(0)) {
        throw new Exception("Skipping commented line in file.");
    }
}

To conclude, I think exceptions work in this case because you expect a specific input, and anything that does not match it (any exception to it) it to be discarded.