1

Is the following code clear and easy to read?

public void createDatabase() throws SQLException, IOException {
    SQLiteDatabase database = dbStore.getDatabase();
    LineNumberReader scriptInputReader = new LineNumberReader(new InputStreamReader(getClass().getResourceAsStream(SCRIPT_CREATE)));
    for(String line; (line = scriptInputReader.readLine()) != null;) {
        database.execSQL(line);
    }
}

I write a lot of "for" loops like the one above. For me it looks clear - it shows the temporary variable ("line") used in the loop, limits it's scope and points out when the loop ends (when "readLine" returns "null"). I wonder if other programmers will hate me for those...

or this one:

    SQLiteDatabase database = dbStore.getDatabase();
    Cursor cursor = database.query("PINS", new String [] {"ID", "X", "Y"}, null, null, null, null, "ID");
    if(cursor.moveToFirst()) {
        for(; !cursor.isAfterLast(); cursor.moveToNext()) {
            (...)
        }
    }
    cursor.close();

Are things like the above just "neat" or already a Java-puzzles?

Improve this question
5
  • Given that I knew a programmer who used a for where most people would use an if I guess you can get away with it, but these examples really look like something to use a while loop on. Commented Jun 28, 2011 at 21:20
  • @fvu: I'm pretty sure if I scored through the Java SDK I could find several "official" examples where a similar for heavy coding style was used. So that's quite subjective I think. Commented Jun 28, 2011 at 21:32
  • I sometimes even write for(;;) { /*...*/ if(something) break; /*...*/ } But it's obviously wrong and I do it only while prototyping. It's like a habit to me ;) Commented Jun 28, 2011 at 21:42
  • better hope the maintenance programmer isn't a psychopath who happens to know your address then :-) Commented Jun 28, 2011 at 21:47
  • @fvu yeah.. WTF per minute ratio on for(;;) is usually very high.... Commented Jun 28, 2011 at 22:13

6 Answers 6

Reset to default
7

I'd opt for:

String line = null;
while((line = scriptInputReader.readLine()) != null) {
  ... do stuff with line
}

This is clear and straightforward.

Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

The initializer to line setting it to null is spurious.
Yes...but...this leaks a bit of memory since line is still assigned after the loop terminates. This also keeps the variable name in the scope after the loop - it may conflict with something else (different variable type) in the same method. And putting the whole thing in additional brackets seems extreme to me ;)
@Kirk Woll -> True, i treat it more as a guide for the person reading it later that I have not forgotten to initialize it, like "Yes I intended to have a non initialized value here".
@Chris -> Yeah.... extra brackets ... I never ever remember which is the sequence of = vs. != operators. ... I a bit rely on Eclipse with this (i have autosave action "remove unnecessary parenthesis" set to do it for me). Maybe a bit lame....
@Jarek -> I've meat the curly brackets to limit the scope of "line" variable: void foo() { { String line = null; while((line = scriptInputReader.readLine()) != null) { ... do stuff with line } } ... there is no line variable here anymore }
|
4

I like what you've done, but I would make one small change:

for(String line = scriptInputReader.readLine(); line != null; line = scriptInputReader.readLine()) {
    database.execSQL(line);
}

This separates the iteration action from the loop termination condition. Also, unlike the "while" version, it limits the scope of the line variable to the loop - narrowing scope as much as possible is good coding practice.

Also, code style checkers usually consider assignments nested within tests as "poor style". To be clear, your code is a bit like this:

for (int i = -1; ++i < max;) { // don't do this: increment action inside condition section
    // some code
}
Improve this answer

3 Comments

Yeah I don't have a problem with only using for loops, but using assignments in conditions without reason is an evil, evil style.
well, the rule for checking assignments in conditions is one of the first things I remove from checkstyle/PMD configuration - precisely because of this use case ;)
I like the idea of keeping the clear loop termination like "line!=null" and clear iteration action "readLine()". I think this looks better than my original loop.
1

I would use a while loop

String line = scriptInputReader.readLine();
while(line != null){
     //do stuff
     line = scriptInputReader.readLine();
}
Improve this answer

Comments

1

I would feel more at ease with while.

The first one is not that bad as it is because it is easy to understand what the loop does, but if you add more logic in the middle of the loop and the operation is complicated it will become more difficult (because people will think:'hey, if he wanted just to read a file he would have used a while, so there must be some trick').

The second one (the for doing at the work, and no code inside the loop) is awful and probably in a not so distant future someone will say: 'Hey, there was a loop here and the contents were removed, but they forgot to remove the loop! I can optimize that by removing the for altogether').

Improve this answer

Comments

0

I think there are two different schools of thought here. The one as shown by Jarek Potiuk that prefers to use for/while loops for different purposes, ie. a for loop should be used when you know the range of your loop in advance (for (;i < arr.length(); i++)) while a while is preferred for unlimited situations.

But then there's the other line of thought that uses only one kind of loop and at that the for loop because it's more versatile. The Java SDK for example uses for loops pretty extensively in unlimited situations (eg linked lists). But then you should really write the for loop like Bohemian does - much clearer.

Improve this answer

Comments

0

Some prefer to use a while-loop when you do not know when the loop will terminate. For example reading through a file.

Jarek Potiuk's solution works, however I prefer something like this:

String line = scriptInputReader.readLine();
while(line != null) 
{
  ... do stuff with line
  line = scriptInputReader.readLine();
}

It is a little bit more code, but I have to agree with Bohemian:

code style checkers usually consider assignments nested within tests as "poor style"

Improve this answer

2 Comments

I am a great fan of code checkers. Really. I introduced them in my company for 25+ people. I love them. As long the code they promote is better.... We have reviewed recently the rules of checkstyle/PMD and removed a bunch of rules which caused more harm than good. assignment in condition was one of them.
@Jarek yeap! and run them in svn hooks to deny all commits violating the code style guidelines, so that it will make a poor programmer's life miserable ;) pozdrawiam :)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.