Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
11  
In either approach, it is worth considering Extract-Till-You-Drop (with well thought of names for your extracted methods) for the code inside your if-blocks. It is certainly a best practice and makes the final top-level function clean and readable either with early-returns (which are easier to spot in a clean + small top-level function) or even with nested-ifs. I've rarely squirmed at either of the two approaches when the thankfully SMALL function's intent is crystal clear and reads like prose. –  shivsky Jan 27 at 15:22
8  
GOTO causes velociraptors. –  Shadur Jan 27 at 19:30
11  
I misread this as Nietzsche vs GOTO... I was genuinely excited to find out what that meant. –  user1167442 Jan 27 at 20:56
9  
I agree; both of these make me twitchy. The solution I would recommend is: take a step back and ask yourself what you are really doing. You are writing a parser without writing a lexer first. I would be inclined to structure this code much more like a traditional compiler. Define a lexical grammer. Decompose the text into a sequence of tokens. Then run a parser over the tokens to produce a data structure. –  Eric Lippert Jan 27 at 23:21
5  
All control structures are just GOTO in disguises, so you should write all your methods without any control structures, no for-loops, no while-loops, no if-conditionals, and no functions. They're all just GOTO and we all know that GOTO is the vilest of all evil. –  Lie Ryan Jan 28 at 5:11

11 Answers 11

up vote 46 down vote accepted

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.

share|improve this answer
8  
I would either call it StringPair or Pair<String,String>. –  200_success Jan 27 at 17:19
3  
The fromString method won't look quite as clean if the LOGGER statements are re-inserted. There's a loss of functionality here. –  Andrew Lazarus Jan 27 at 22:19
    
@AndrewLazarus: I do not consider Logger statements "functionality", I also fail to see the need to reinsert all of these statements. In my opinion it should be obvious why the function returned what it returned by looking at the input. If it is not-obvious from looking at the input, then there's something wrong with the function. You could achieve something along these lines by splitting the if condition and utilizing the else parts. That way you get logging but without the need for finer grained messages (it's either: "Yo, this is malformed" or "Already exists"). –  Bobby Jan 27 at 22:40
2  
Please, please don't use null to signify invalid lines. Either throw IllegalArgumentException or use Guava's / SE8's Optional<T> . The OP doesn't specify what the file looks like, but null is really not the thing for this. –  TC1 Jan 28 at 12:13
1  
There are however some issues with this code: 1. As other's mentioned, null is a bad way to signal validation errors. 2. Pair.fromString does both parsing and validation. By returning null you lose the information about the validation error (i.e. the "malformed line" message). So the code is functionally different, regardless of whether you think that this information is unimportant. 3. You didn't actually answer OP's question. Well, implicitly you did, because you chose nested if, but guard statements are IMHO more readable (i.e. simply writing if (pair == null) continue;). –  Groo Jan 29 at 8:28

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.

share|improve this answer
8  
I'm not sure why you object so strongly to continue. I'd be curious to see what arguments you come up with for a "continue statement considered harmful" essay. –  200_success Jan 27 at 17:23
5  
@JeffGohlke - Everything I've ever seen about continue/break has only said that the labeled version should be avoided. Avoiding the unlabeled form inside a loop is like avoiding return statements. –  Bobson Jan 27 at 19:23
3  
@Bobson I agree with the analogy completely. The question could be restructured to ask about having lots of return statements ("return early, return often") or having lots of nested conditions and fewer returns. Same idea. –  asteri Jan 27 at 19:31
2  
@IraBaxter - If your code is complex enough that you can't tell you're in a loop, then you really need to refactor it. And since the unlabeled versions only apply to the current loop, it should be trivial to tell where they go. –  Bobson Jan 28 at 12:36
2  
@IraBaxter - Got a link to source that? I can't find anything on google, but I'm not sure what terms to use. –  Bobson Jan 28 at 15:05

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

share|improve this answer

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.

share|improve this answer
    
Thank you for this post. This really gave me an epiphany moment concerning the code. –  asteri Jan 28 at 4:39
1  
@OldCurmudgeon The problem with this approach is you separate validation and processing. Which would lead to code duplication. e.g. // Do stuff with valid lines. would also contain a String.split("="). –  abuzittin gillifirca Jan 28 at 8:40
1  
@abuzittingillifirca - That miniscule loss is balanced by a significant gain in code quality. It also suggests that you should use a new class to handle the String so you can retain the parts. –  OldCurmudgeon Jan 28 at 9:00
    
Good call on the Java8 streams .... looks interesting. And thanks for the 'good call' as well. You will probably find that this .filter(s -> s.length() > 0) is better written as .filter(s -> !s.isEmpty())... Nice Answer! (good thing you edited. I have votes now!) –  rolfl Jan 31 at 0:53
    
@rolfl - Thanks (again) for the useful feedback. isEmpty deployed. –  OldCurmudgeon Jan 31 at 10:11

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.

share|improve this answer
4  
Even though the question is more about code style, this is a case of where the question can be "X or Y?" and the answer is "Z!". A reminder to always think outside the box. +1 for the edited version of this answer. –  Simon André Forsberg Jan 27 at 21:00

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?

share|improve this answer

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
share|improve this answer
    
I was just about to suggest a functional approach too. The problem is basically passing some input through a series of filters, which is modeled perfectly in a functional way. –  d11wtq Jan 28 at 9:47
1  
This is also a very good idea. Thank you. I might actually end up going with this. –  asteri Jan 28 at 15:34
2  
Java 8 can help here :) –  Silviu Burcea Jan 29 at 8:55
    
@SilviuBurcea Can you explain? My production environment is just getting Java 7 this year, so I'm still busy trying to master things like lambda and try-with-resources blocks. Haha –  asteri Jan 29 at 21:39
    
@JeffGohlke Java 8 introduced Streams and some other utilities, like Files.lines(path) that returns a Stream. Map and filter are applied on streams. You can then map every line to something(even a bean), then you can filter them on the fly. –  Silviu Burcea Jan 30 at 7:57

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
2  
A comment is too routine to justify the use of an exception. The variable-already-assigned situation might be handled that way. Still, catching all Exceptions makes me uneasy. To trace the intended flow, I have to examine every single function call in the try block and brainstorm ideas about what kinds of exceptions might be thrown. It reminds me of this code. –  200_success Jan 31 at 0:13

protected by Mat's Mug Jan 28 at 16:00

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.