Nesting versus GOTO: which is better to avoid?

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 and values is a Map):

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 bac

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:

• It's a method of its own, easily reusable, easily testable.

• 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.

StackExchange Code Review Q#40162, answer score: 62