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.

The specific issue is this: I am writing to a file and want to output a new line after each line written. If I use a normal loop without any further checks, this will create a blank line at the very end of the file for no reason. So I need to do this every time except the last one.

Despite the fact that this is a specific problem, I'm actually looking for a better general coding practice to deal with scenarios like this, since it isn't the first and won't be the last time I have to deal with it.

Here's my unsatisfying solution:

//Write contents to the file
BufferedWriter writer = new BufferedWriter(new FileWriter(file));
for(int i = 0; i < lines.size(); i++) {
    writer.write(lines.get(i));
    if(i < lines.size() - 1) writer.newLine();
}

It seems wasteful to check the conditions twice through each iteration of the loop, and I feel like there ought to be a better way to accomplish what I want without the vague code smell. Anyone have any cool tips or tricks to make this more elegant?

It also prevents me from using an enhanced for loop, which makes me sad.

lines is a List<String>.

Also, for those saying I should just join all the Strings with \n, that's not an adequate solution. Firstly, it doesn't actually address the general coding practice. Secondly, when writing to a file with a BufferedWriter, it's important to use newLine() rather than writing a \n.

share|improve this question
7  
*Somebody has to say this... a text file is a series of lines. A line includes a newline terminator. A file that does not have a newline as its final character is not a text file. A file that ends in a blank line has 2 consecutive newlines as its last 2 characters: one to terminate the next-to-last line and one to terminate the empty line. "Text files" with an unterminated last line are a disease. Don't catch it! –  Wumpus Q. Wumbley Dec 13 '13 at 19:36
3  
@WumpusQ.Wumbley In what circumstances would that ever be necessary? I'm not saying there are none, I just don't know of any. Arbitrarily declaring a format that all text files must follow seems overly ambitious. –  Daniel Cook Dec 13 '13 at 20:15
2  
@WumpusQ.Wumbley It may be a windows thing, from wikipedia Text file : "MS-DOS and Windows use a common text file format, with each line of text separated by a two-character combination: CR and LF, which have ASCII codes 13 and 10. It is common for the last line of text not to be terminated with a CR-LF marker, and many text editors (including Notepad) do not automatically insert one on the last line." –  Daniel Cook Dec 13 '13 at 20:17
    
@DanielCook It's not just a Windows thing. Many Unix shell utilities, especially the older ones, behave unpredictably if the last line doesn't end in LF. On the other hand, sometimes the absence of a final LF is essential, as when using PHP to generate XML - if the PHP source file has a LF after the final ?>, it will be copied to the output and probably wind up in a place where the XML parser doesn't like it. So it's not by any means a hard rule. –  Zack Dec 13 '13 at 20:31
1  
@WumpusQ.Wumbley - One of the 16-bit Visual C++ compilers had a bug that would manifest when the source file did not end with CR/LF. But I haven't encountered anything else with such an issue in the past 15 years. –  GalacticCowboy Dec 13 '13 at 20:45

10 Answers 10

up vote 21 down vote accepted

I presume lines is a Collection of some sort. One option that has slightly less of a smell (although it still is odoriferous), is to use an iterator, which will essentially do the same work, but will be more readable:

for (Iterator<String> it = lines.iterator(); it.hasNext();) {
    writer.write(it.next());
    if (it.hasNext()) {
        writer.newline();
    }
}

As I say, all this does is make it more readable....

Other options are to duplicate the write - once in the loop, and then the last one outside the loop:

if (!lines.isEmpty()) {
    int limit = lines.size() - 1;
    for (int i = 0; i < limit; i++) {
        ....
    }
    writer.write(lines.get(limit));
}

EDIT: @tomdemuyt suggested reversing the newline to happen only after the first line as follows:

if (!lines.isEmpty()) {
    writer.write(lines.get(0));
    // start index at 1 instead of 0.
    for (int i = 1; i < lines.size(); i++) {
        writer.newline();
        writer.write(lines.get(limit));
    }
}
share|improve this answer
5  
+1 for pulling the last write out of the loop. –  Cruncher Dec 13 '13 at 15:05
    
Yep, lines is a List<String>. Nice. I like the idea of performing the last write outside of the loop rather than constantly checking inside of the loop. Still seems vaguely funky to me, but better than what I've got now. Thanks. :) –  Jeff Gohlke Dec 13 '13 at 15:15
8  
I tend to write the first one without newline, then write newline + nextline for the rest, starting loop from 1. –  konijn Dec 13 '13 at 15:22
1  
@tomdemuyt - put that in an answer - it's a good solution. –  rolfl Dec 13 '13 at 15:23
    
@tomdemuyt That's actually probably what I'll end up doing, now that you mention it. You definitely should post that as an answer. –  Jeff Gohlke Dec 13 '13 at 15:34

This case is usually concerning joining strings. There's a method for that in Apache Commons Lang:

StringUtils.join(lines, "\n");

Also here's a pattern that can be used with a foreach loop

StringBuilder buf = new StringBuilder();
for (String line : lines) {
    if(buf.length() > 0) {
        buf.append("\n");
    }
    buf.append(line);
}
share|improve this answer
    
I am not sure this code solves the problem ... !lines.isEmpty() will return true on every run through the loop. –  rolfl Dec 13 '13 at 15:39
    
My mistake: I meant buf.isEmpty() –  rzymek Dec 13 '13 at 15:46

You can do it by just removing the last seperator when you are done:

CharSequence concatSep(Iterable<?> items, CharSequence separator){
    if(!lines.iterator().hasNext()) return "";

    StringBuilder b = new StringBuilder();
    for(Object item: items)
        b.append(item.toString()).append(separator);
    return b.delete(b.length() - separator.length(), b.length());
}

where separator is the desired item seperator, whether newline, comma, semicolon, tab, or more than one character.

In your case: concatSep(lines, System.getProperty("line.seperator")).

share|improve this answer
    
This does not work if lines.Length == 0. –  Matt Dec 14 '13 at 5:59
    
@Matt Fixed, also put them in functions. –  AJMansfield Dec 14 '13 at 17:23

For the general case you can either pull the first or last line out of the loop, or move the loop exit into the middle of the loop using a break statement - modifying rolfl's example:

Iterator<String> it = lines.iterator()
if (it.hasNext()) {
    while (true) {
        writer.write(it.next());
        if (!it.hasNext()) 
            break;
        writer.newline();
    }
}

'Structured programming with goto statements' is a classic paper on handling non-standard looping cases.

share|improve this answer
1  
This pattern is my favorite because: a. It doesn't require you to duplicate code - "writer.write" appears only once. b. It doesn't require you to duplicate tests inside the loop - the loop exit test is performed only once, in the middle of the loop. –  Erel Segal Halevi Dec 15 '13 at 15:24
    
A better idiom for this needs to be added to java: just let the user have both a block before and after the same while. Like this: do { doSomething(); } while (condition()) { doSomethingElse(); } –  AJMansfield Dec 17 '13 at 2:39

A small twist on @rolfls' answer:

BufferedWriter writer = new BufferedWriter(new FileWriter(file));
if ( lines.size() > 0 ) {
    writer.write(lines.get(0));
}
for (int i = 1; i < lines.size(); i++) {
    writer.newLine();
    writer.write(lines.get(i));
}

Exact same idea though, move the extra check outside of the loop.

share|improve this answer
    
Hm... are you coming from JavaScript? I don't think if(lines.size()) will compile. –  Jeff Gohlke Dec 13 '13 at 15:51
    
Indeed -- It's the logic I'd like to express, not the syntax ;) –  smassey Dec 13 '13 at 16:05
    
FWIW, I personally feel this may be the best approach. I was about to post the same idea, when I finally noticed someone already had. The reason I like this better is that there is only one if check done at the beginning. Most of the other solutions require doing some kind of comparison each run through. In this case, we can eliminate unnecessary comparisons through the for loop. My two cents... –  Charlie74 Dec 13 '13 at 21:12
1  
lines.size() > 0 is lame and potentially harmful (if lines is a singly linked list, knowing its size might be O(n)). Use lines.nonEmpty() or !lines.isEmpty() if the earlier is not available. –  scand1sk Dec 14 '13 at 15:44

why not reverse it: write a newline first except on the first line:

boolean newline = false;
for(int i = 0; i < lines.size(); i++) {
    if(newline) writer.newLine();
    else newline = true;
    writer.write(lines.get(i));
}
share|improve this answer

There are two issues in your code:

  1. As you noticed, the if is checked at each loop although you know that it will be invalidated only on the last loop. A good way to avoid the problem is to treat the first or last element of your list specifically before (or after) entering the loop. Treating the first element separately is much easier.

    Note that your if check may be quite cheap if you assume that calculating size is constant-time. (Note that size could be calculated once before the loop.) The optimization will probably be completely negligible (especially since you are performing costly I/O in the loop).

  2. Maybe more subtle: if lines is a List, is may not be indexed (e.g., lines is a LinkedList). Calling lines.get(i) may then be O(i), and your whole loop be O(n²) although O(n) is doable. Using an Iterator as @rolfl suggested is the best way to avoid this problem. It may or may not improve readibility depending on your experience, but it will surely improve performance drastically depending on the nature of your List.

BTW, this problem is solved generically in the very Java API: look for the implementation of toString in AbstractCollection (just replace separators with your own, and remove the test for e == this which is quite specific):

public String toString() {
    Iterator<E> it = iterator();
    if (! it.hasNext())
        return "[]";

    StringBuilder sb = new StringBuilder();
    sb.append('[');
    for (;;) {
        E e = it.next();
        sb.append(e == this ? "(this Collection)" : e);
        if (! it.hasNext())
            return sb.append(']').toString();
        sb.append(',').append(' ');
    }
}

I would be surprised if a more general implementation with user-definable separators would not to be found in Apache Commons or Google Guava.

Anyway, here is the final code, using BufferedWriter instead of StringBuilder:

private static void <E> write(BufferedWriter writer, List<E> lines) {
    Iterator<E> it = lines.iterator();
    if (!it.hasNext()) return;

    for (;;) {
        E e = it.next();
        writer.write(e);
        if (!it.hasNext()) return;
        writer.newLine();
    }
}
share|improve this answer
    
You're right, I knew I could simply join the various Strings together with \n as you suggest. But when writing to a file with BufferedWriter, you're supposed to use newLine() instead because it's system agnostic. –  Jeff Gohlke Dec 14 '13 at 17:16
    
Well, you can still use the above structure, replacing references to StringBuilder by BufferedWriter. I edited my answer above to include the final code. –  scand1sk Dec 14 '13 at 21:58

Just a little simpler:

BufferedWriter writer = new BufferedWriter(new FileWriter(file));
for(int i = 0; i < lines.size(); i++) {
    if(i > 0) writer.newLine();
    writer.write(lines.get(i));
}
share|improve this answer

Definitely, definitely take the if statement out of the loop. People always talk about "the optimizer" but optimizers are different, and whatever you can do to help it is probably a good idea.

//Write contents to the file
BufferedWriter writer = new BufferedWriter(new FileWriter(file));
for(int i = 0; i < lines.size() - 1; i++) {
    writer.write(lines.get(i));
    writer.newLine();
}

// Write the last one without extra newline
if( lines.size() )
  writer.write(lines.get(lines.size()-1));
share|improve this answer
    
This does not work if lines.Length == 0. –  Matt Dec 14 '13 at 6:01
    
@Matt Good point. Fixed. –  bobobobo Dec 14 '13 at 14:03
BufferedWriter writer = new BufferedWriter(new FileWriter(file));
   int i =0;
   for(;i < lines.size()-1; i++)
   {
     writer.write(lines.get(i));
     writer.newLine();
   }
   if(lines.size()>0)
   {
   writer.write(lines.get(i));
   }

This way you can avoid the conditional statement every time, keeping your code the same.

share|improve this answer

protected by rolfl Dec 18 '13 at 0:25

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.