46

We have a Java Application that has a few modules that know to read text files. They do it quite simply with a code like this:

BufferedReader br = new BufferedReader(new FileReader(file));  
String line = null;  
while ((line = br.readLine()) != null)  
{  
   ... // do stuff to file here  
} 

I ran PMD on my project and got the 'AssignmentInOperand' violation on the while (...) line.

Is there a simpler way of doing this loop other than the obvious:

String line = br.readLine();  
while (line != null)  
{  
   ... // do stuff to file here  
   line = br.readLine();  
} 

Is this considered a better practice? (although we "duplicate" the line = br.readLine() code?)

4
  • Nice BufferedReaderIterator. I had to replace r.mark(1) with r.mark(2), otherwise would have an "invalid mark" about 100 lines into a large file. Don't understand why.
    – user1202927
    Feb 10, 2012 at 20:46
  • 5
    How about a for loop? for (String line = br.readLine(); line != null; line = br.readLine()) { ... } Aug 5, 2015 at 19:11
  • Does this answer your question? Reading a plain text file in Java
    – tevemadar
    Feb 13 at 16:11
  • And perhaps stackoverflow.com/questions/23276407/… for the streaming one.
    – tevemadar
    Feb 13 at 16:12

8 Answers 8

38

I know is an old post but I just had the same need (almost) and I solve it using a LineIterator from FileUtils in Apache Commons. From their javadoc:

LineIterator it = FileUtils.lineIterator(file, "UTF-8");
try {
    while (it.hasNext()) {
    String line = it.nextLine();
    // do something with line
    }
} finally {
    it.close();
}

Check the documentation: http://commons.apache.org/proper/commons-io/javadocs/api-release/org/apache/commons/io/LineIterator.html

5
  • Thanks, it can come in handy in the future.
    – RonK
    Aug 23, 2012 at 17:42
  • 1
    In case anyone needs this dependency (FileUtils) from Maven : <dependency> <groupId>commons-io</groupId> <artifactId>commons-io</artifactId> <version>2.4</version> </dependency>
    – blue-sky
    Dec 17, 2013 at 16:26
  • I'm fairly certain you can do this same kind of thing with a Scanner too.
    – anon
    Jan 17, 2016 at 4:11
  • Pity that it doesn't implement AutoClosable :( Jul 27, 2017 at 15:57
  • It seems like you should be able to use this like: for(String line: FileUtils.lineIterator(file,"UTF-8")) { /*do something */ }. But sadly that wont close the iterator cleanly. Dec 21, 2018 at 0:57
27

The support for streams and Lambdas in java-8 and Try-With-Resources of java-7 allows you to achive what you want in more compact syntax.

Path path = Paths.get("c:/users/aksel/aksel.txt");

try (Stream<String>  lines = Files.lines(path)) {
    lines.forEachOrdered(line->System.out.println(line));
} catch (IOException e) {
    //error happened
}
2
  • 2
    Can also shorten the lambda to a method reference: lines.forEachOrdered(System.out::println)
    – Alex
    Jun 27, 2016 at 15:49
  • I like this one, but I wanted to set boolean values while iterating the file to signify that I'd reached the area of interest, but it said I cannot use non-final variables in the body of the loop so it was a no-go for me. I used @rolfl's answer instead stackoverflow.com/a/22351492/529256
    – Craig
    Nov 15, 2021 at 16:18
22

I generally prefer the former. I don't generally like side-effects within a comparison, but this particular example is an idiom which is so common and so handy that I don't object to it.

(In C# there's a nicer option: a method to return an IEnumerable<string> which you can iterate over with foreach; that isn't as nice in Java because there's no auto-dispose at the end of an enhanced for loop... and also because you can't throw IOException from the iterator, which means you can't just make one a drop-in replacement for the other.)

To put it another way: the duplicate line issue bothers me more than the assignment-within-operand issue. I'm used to taking in this pattern at a glance - with the duplicate line version I need to stop and check that everything's in the right place. That's probably habit as much as anything else, but I don't think it's a problem.

2
  • I'm curious what you think about creating a decorator as a convenience mechanism that abstracts the semantics of the iteration so that you can use a foreach loop (see my response with a rough suggestion below)... Jan 13, 2011 at 6:45
  • @Mark E: It's not as neat as the C# version, but not bad - except for exceptions. I'll comment on your answer and edit mine.
    – Jon Skeet
    Jan 13, 2011 at 7:09
22

I routinely use the while((line = br.readLine()) != null) construct... but, recently I came accross this nice alternative:

BufferedReader br = new BufferedReader(new FileReader(file));

for (String line = br.readLine(); line != null; line = br.readLine()) {
   ... // do stuff to file here  
}

This is still duplicating the readLine() call code, but the logic is clear, etc.

The other time I use the while(( ... ) ...) construct is when reading from a stream in to a byte[] array...

byte[] buffer = new byte[size];
InputStream is = .....;
int len = 0;
while ((len = is.read(buffer)) >= 0) {
    ....
}

This can also be transformed in to a for loop with:

byte[] buffer = new byte[size];
InputStream is = .....;
for (int len = is.read(buffer); len >= 0; len = is.read(buffer)) {
    ....
}

I am not sure I really prefer the for-loop alternatives.... but, it will satisfy any PMD tool, and the logic is still clear, etc.

1
  • 1
    Nice approach! You can also wrap the BufferedReader instance creation with try-with-resources statement if it's used with Java 7, it will reduce the scope of the variable and add automatically close the reader all lines are handled.
    – Pavel
    Sep 27, 2016 at 7:52
4

Based on Jon's answer I got to thinking it should be easy enough to create a decorator to act as a file iterator so you can use a foreach loop:

public class BufferedReaderIterator implements Iterable<String> {

    private BufferedReader r;

    public BufferedReaderIterator(BufferedReader r) {
        this.r = r;
    }

    @Override
    public Iterator<String> iterator() {
        return new Iterator<String>() {

            @Override
            public boolean hasNext() {
                try {
                    r.mark(1);
                    if (r.read() < 0) {
                        return false;
                    }
                    r.reset();
                    return true;
                } catch (IOException e) {
                    return false;
                }
            }

            @Override
            public String next() {
                try {
                    return r.readLine();
                } catch (IOException e) {
                    return null;
                }
            }

            @Override
            public void remove() {
                throw new UnsupportedOperationException();
            }

        };
    }

}

Fair warning: this suppresses IOExceptions that might occur during reads and simply stops the reading process. It's unclear that there's a way around this in Java without throwing runtime exceptions as the semantics of the iterator methods are well defined and must be conformed to in order to use the for-each syntax. Also, running multiple iterators here would have some strange behavior; so I'm not sure this is recommended.

I did test this, though, and it does work.

Anyway, you get the benefit of for-each syntax using this as a kind of decorator:

for(String line : new BufferedReaderIterator(br)){
    // do some work
}
2
  • I suspect this won't compile, because readLine could throw IOException. The Iterator interface won't allow that, so you'd have to wrap it in an unchecked exception, at which point it's starting to look less and less like the original code :(
    – Jon Skeet
    Jan 13, 2011 at 7:10
  • @Jon: you're right, and unfortunately I'm pretty sure there's no way around hiding the exceptions to get the semantics. While it's convenient the payoff seems dismal. Feb 2, 2011 at 15:42
4

I'm a bit surprised the following alternative was not mentioned:

while( true ) {
    String line = br.readLine();
    if ( line == null ) break;
    ... // do stuff to file here
}

Before Java 8 it was my favorite because of its clarity and not requiring repetition. IMO, break is a better option to expressions with side-effects. It's still a matter of idioms, though.

1
  • 1
    "while (true)" is generally perceived as bad practice.
    – psychosys
    Jun 28, 2023 at 18:06
3

Google's Guava Libraries provide an alternative solution using the static method CharStreams.readLines(Readable, LineProcessor<T>) with an implementation of LineProcessor<T> for processing each line.

try (BufferedReader br = new BufferedReader(new FileReader(file))) {
    CharStreams.readLines(br, new MyLineProcessorImpl());
} catch (IOException e) {
    // handling io error ...
}

The body of the while loop is now placed in the LineProcessor<T> implementation.

class MyLineProcessorImpl implements LineProcessor<Object> {

    @Override
    public boolean processLine(String line) throws IOException {
        if (// check if processing should continue) {
            // do sth. with line
            return true;
        } else {
            // stop processing
            return false;
        }
    }

    @Override
    public Object getResult() {
        // return a result based on processed lines if needed
        return new Object();
    }
}
1

AssignmentInOperand is a controversial rule in PMD, the reason of this rule is: "this can make code more complicated and harder to read" (please refer http://pmd.sourceforge.net/rules/controversial.html)

You could disable that rule if you really want to do it that way. In my side I prefer the former.

1
  • 1
    Or put a comment // NOPMD Apr 7, 2014 at 11:31

Your Answer

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

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