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.

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.

share|improve this question
    
You are mainly asking for a based-opinion answer, in my case the first approach has proven to be more clear when I need to choose between both, but it is just my case. –  morgano Mar 12 at 9:32
    
I assume that last reader.readLine(); should be line = reader.readLine();? –  tobias_k Mar 12 at 9:37
    
tobias_k: yes, thanks. I've fixed it. morgano: well, it is a question about coding guidelines, and most such questions are opinion-based. Still, I would like to know if majority prefers one of these options or if there is no strong preference. Thanks for you reply! –  mkalkov Mar 12 at 9:50
3  
I consider demoA to be idiomatic for Java. That said, there's an old SO question on the same topic: stackoverflow.com/questions/4677411/… Especially the highest-rated (not accepted) answer is interesting. –  Sebastian Redl Mar 12 at 10:52
2  
I don't write String line = null; but String line;. –  johnchen902 Mar 12 at 12:49

5 Answers 5

up vote 45 down vote accepted

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.

share|improve this answer
6  
+1 for for loop. I hadn't thought about it before but that is clear and avoids having line outside of the loop scope. –  Emily L. Mar 12 at 10:40
1  
I also like the for loop solution. It is dense, readable and does not draw attention away from doSomething(line). –  mkalkov Mar 12 at 11:59
4  
@FabioF. I do not create a new String each time, that happens anyway because of readLine. It is however considered a best practice to restrict the visibility of variables to the smallest possible scope. Here, this means the line should not be declared outside of the loop. Not reassigning variables and other aspects of immutability are a good habit I picked up when doing functional programming: This makes code easier to understand as the variable name and its value can be used interchangeably (“referential transparency”), there is no mutating state I have to keep in mind. –  amon Mar 12 at 13:16
1  
@FabioF. While that would restrict the visibility properly, it adds an unnecessary level of indentation. And we might just as well use an equivalent for (String line; ...;) { ... } instead – all of the advantages, none of the downsides. –  amon Mar 12 at 13:24
2  
I like the last solution best. It's often a good idea to avoid having loop exit points which are separated by code that has meaningful side-effects, but if the natural location for the loop exit point is in the middle, it's better to use a while/break construct than duplicate the code which should precede the loop exit point. –  supercat Mar 12 at 15:58

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

share|improve this answer
2  
Why are you using this iterator manually instead of inside a foreach loop? –  Phoshi Mar 13 at 15:05
3  
@Phoshi: unfortunately foreach can't use Iterator, it needs and Iterable. –  palacsint Mar 13 at 15:13
2  
@palacsint: Ah, of course. I guess my question is then why on earth LineIterator doesn't implement Iterable, but I suppose that isn't something you can answer. –  Phoshi Mar 13 at 15:32
1  
@Phoshi: You can pass any reader (with any stream) to the LineIterator. Some of them might not support reset (and can be read only once, like a network socket). Rereading multiple times requires creating multiple iterators with Iterable.iterator(). –  palacsint Mar 13 at 15:40
1  
(Not to mention that using multiple LineIterators for concurrency would possibly be worse than a LineIterable class as each of those LineIterators could close the Reader, which would invalidate any other LineIterators on that Reader even if it were one supporting mark/reset/skip and the iterators were subclassed to keep track of their positions independently [though you could also override LineIterator.close() to not actually call Reader.close() until all iterators for that Reader have closed/been consumed - perhaps a concurrent Map<Reader, Integer> for the counts?]). –  JAB Mar 13 at 17:11

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.

share|improve this answer

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!

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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