Home » Nesting versus GOTO: which is better to avoid?

Nesting versus GOTO: which is better to avoid?

Solutons:


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.

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.

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.

Related Solutions

How to delete a non-empty directory in Terminal?

Use the below command : rm -rf lampp It deletes all files and folders contained in the lampp directory. In case user doesn't have the permission to delete the folder: Add sudo at the beginning of the command : sudo rm -rf folderName Otherwise, without sudo you...

mysql: Show GRANTs for all users

Nothing built-in. You have two options though: Use common_schema's sql_show_grants view. For example, you can query: SELECT sql_grants FROM common_schema.sql_show_grants; Or you can query for particular users, for example: SELECT sql_grants FROM...

Configurable values to MDB annotations

You can externalise the annotations into the ejb-jar.xml that you deploy in the META-INF of your jar file as follows: <?xml version="1.0" encoding="UTF-8"?> <ejb-jar version="3.0"> <enterprise-beans> <message-driven>...

How do I select which Apache MPM to use?

There are a number of MPM modules (Multi-Processing Modules), but by far the most widely used (at least on *nix platforms) are the three main ones: prefork, worker, and event. Essentially, they represent the evolution of the Apache web server, and the different...

Using var self = this or .bind(this)? [closed]

Things that would favor var self = this; bind isn't supported in IE8 and Safari5. If you aim to build a library or code that supports legacy browsers, then var self = this would be more cross-browser friendly. Sometimes, callbacks are bound to a certain context...

What is the difference between SSL vs SSH? Which is more secure?

SSL and SSH both provide the cryptographic elements to build a tunnel for confidential data transport with checked integrity. For that part, they use similar techniques, and may suffer from the same kind of attacks, so they should provide similar security (i.e....

How can I stop applications and services from running?

First Things First You may have some misconceptions about how Android works and what's really happening when a service is running or an app is in the background. See also: Do I really need to install a task manager? Most apps (e.g., ones you launch manually)...

How do I reset a lost administrative password?

By default the first user's account is an administrative account, so if the UI is prompting you for a password it's probably that person's user password. If the user doesn't remember their password you need to reset it. To do this you need to boot into recovery...

How can I use environment variables in Nginx.conf

From the official Nginx docker file: Using environment variables in nginx configuration: Out-of-the-box, Nginx doesn't support using environment variables inside most configuration blocks. But envsubst may be used as a workaround if you need to generate your...

Difference between .bashrc and .bash_profile

Traditionally, when you log into a Unix system, the system would start one program for you. That program is a shell, i.e., a program designed to start other programs. It's a command line shell: you start another program by typing its name. The default shell, a...

Custom query with Castle ActiveRecord

In this case what you want is HqlBasedQuery. Your query will be a projection, so what you'll get back will be an ArrayList of tuples containing the results (the content of each element of the ArrayList will depend on the query, but for more than one value will...

What is the “You have new mail” message in Linux/UNIX?

Where is this mail? It's likely to be in the spool file: /var/mail/$USER or /var/spool/mail/$USER are the most common locations on Linux and BSD. (Other locations are possible – check if $MAIL is set – but by default, the system only informs you about...

How can I find the implementations of Linux kernel system calls?

System calls aren't handled like regular function calls. It takes special code to make the transition from user space to kernel space, basically a bit of inline assembly code injected into your program at the call site. The kernel side code that "catches" the...

Is a composite index also good for queries on the first field?

It certainly is. We discussed that in great detail under this related question: Working of indexes in PostgreSQL Space is allocated in multiples of MAXALIGN, which is typically 8 bytes on a 64-bit OS or (much less common) 4 bytes on a 32-bit OS. If you are not...