Is it OK to use while ((line = r.readLine()) != null) construct? [closed]

Question

I want to refactor the following code because I don't feel comfortable about using assignment inside comparison operator. It looks like pretty idiomatic C, but do you think this is a good practice in Java?

private void demoA(BufferedReader reader) throws IOException {
    String line = null;
    while ((line = reader.readLine()) != null) {
        doSomething(line);          
    }
}

Here is an alternative.

private void demoB(BufferedReader reader) throws IOException {
    String line = reader.readLine();
    while (line != null) {
        doSomething(line);
        line = reader.readLine();
    }
}

UPDATE: I've stumbled across a similar question asked couple years ago. It seems that opinions on whether it's OK or not are divided. However, both Guava and Commons IO provide alternative solutions for this issue. If I had any of these libs in the current project, I'd probably use them instead.

Answer 1

Assignment inside a condition is ok in this case, as the assignment is surrounded by an extra pair of parentheses – the comparison is obviously != null, there is no chance that we wanted to type line == reader.readLine().

However, a for loop might actually be more elegant here:

for (String line = reader.readLine(); line != null; line = reader.readLine()) {
    doSomething(line);
}

Alternatively, we could do this which also restricts the scope of line as with the for-loop, and additionally eliminates unnecessary repetition:

while (true) {
    final String line = reader.readLine();
    if (line == null) break;

    doSomething(line);
}

I like this solution most because it doesn't mutate any variables.

Answer 2

You could increase the abstraction level of the code a little bit with an iterator-like pattern and the same time you could reuse an existing library (with the experience of the authors) for that: Apache Commons IO LineIterator. It would replace the null check to a little bit readable hasNext()/nextLine().

Using an iterator hides an unnecessary detail: the reader returns null when there is no more data. The hasNext() method is closer to the (English) language, the code is easier to read. You still can check the details inside LineIterator if you need that but usually readers/maintainers happier with a higher level overview of the method which is easier to understand. (This answer and question contain an expressive example.)

A sample method:

private void demoC(BufferedReader reader) throws IOException {
    final LineIterator it = new LineIterator(reader);
    try {
        while (it.hasNext()) {
            String line = it.nextLine();
            // do something with line
        }
    } finally {
        it.close();
    }
}

See also: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

Answer 3

Instead of wrapping the reader in an iterator, you could also wrap it in an Iterable that then returns the iterator.

It would allow you to write the following

for (String line: linesOf(reader)) {
     // ...
}

which makes very clean code.

Answer 4

In my opinion, demoA() is fine just as it is.

Assignment as a side effect within a test is normally frowned upon, but this usage is an excellent example of why the language feature exists. It's compact, non-repetitive, idiomatic, and efficient. Use it, and don't feel guilty about it!

Answer 5

Please realize that

if (cond(var = expr))

can usually be rewritten as

var = expr;
if (cond(var)) ...

and

while (cond(var = expr))

can always be rewritten as

for (var = expr; cond(var); var = expr)

without even affecting the meaning of break; or continue; in the loop.

So there is quite rarely a definite need for cramming assignments into conditionals' conditions.

Answer 6

You can probably do something like this as well:

Files.lines(path)

It gets all the lines from the file as a stream, then you can sort the string based on your logic and then collect the same in a list and write to the output file.

This answer gives you a functional programming approach. Most part of the method is self-explanatory.

Files.lines(Paths.get(path)).map(--Your business logic--);

There are various functions available in the steam API which can make your processing easy.