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 String
s 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
.
13 Answers 13
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: @konijn 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));
}
}
-
\$\begingroup\$ Yep,
lines
is aList<String>
. Nice. I like the idea of performing the lastwrite
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. :) \$\endgroup\$asteri– asteri2013年12月13日 15:15:47 +00:00Commented Dec 13, 2013 at 15:15 -
9\$\begingroup\$ I tend to write the first one without newline, then write newline + nextline for the rest, starting loop from 1. \$\endgroup\$konijn– konijn2013年12月13日 15:22:30 +00:00Commented Dec 13, 2013 at 15:22
-
1\$\begingroup\$ @tomdemuyt - put that in an answer - it's a good solution. \$\endgroup\$rolfl– rolfl2013年12月13日 15:23:57 +00:00Commented Dec 13, 2013 at 15:23
-
\$\begingroup\$ @tomdemuyt That's actually probably what I'll end up doing, now that you mention it. You definitely should post that as an answer. \$\endgroup\$asteri– asteri2013年12月13日 15:34:44 +00:00Commented Dec 13, 2013 at 15:34
-
\$\begingroup\$ Reason I did not is because in my case I always have minimum 2 lines, now the code can fail if there is only 1 line, and then it gets ugly again. \$\endgroup\$konijn– konijn2013年12月13日 21:03:16 +00:00Commented Dec 13, 2013 at 21:03
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);
}
-
\$\begingroup\$ I am not sure this code solves the problem ...
!lines.isEmpty()
will return true on every run through the loop. \$\endgroup\$rolfl– rolfl2013年12月13日 15:39:12 +00:00Commented Dec 13, 2013 at 15:39 -
\$\begingroup\$ My mistake: I meant
buf.isEmpty()
\$\endgroup\$rzymek– rzymek2013年12月13日 15:46:02 +00:00Commented Dec 13, 2013 at 15:46 -
\$\begingroup\$ That could have severe memory-allocation implications if
lines
is large. \$\endgroup\$Toby Speight– Toby Speight2024年04月25日 06:52:49 +00:00Commented Apr 25, 2024 at 6:52
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"))
.
-
\$\begingroup\$ This does not work if lines.Length == 0. \$\endgroup\$Matt– Matt2013年12月14日 05:59:26 +00:00Commented Dec 14, 2013 at 5:59
-
\$\begingroup\$ @Matt Fixed, also put them in functions. \$\endgroup\$AJMansfield– AJMansfield2013年12月14日 17:23:36 +00:00Commented Dec 14, 2013 at 17:23
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.
-
\$\begingroup\$ 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... \$\endgroup\$Charlie74– Charlie742013年12月13日 21:12:25 +00:00Commented Dec 13, 2013 at 21:12 -
2\$\begingroup\$
lines.size() > 0
is lame and potentially harmful (iflines
is a singly linked list, knowing its size might beO(n)
). Uselines.nonEmpty()
or!lines.isEmpty()
if the earlier is not available. \$\endgroup\$scand1sk– scand1sk2013年12月14日 15:44:36 +00:00Commented Dec 14, 2013 at 15:44
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.
-
1\$\begingroup\$ 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. \$\endgroup\$Erel Segal-Halevi– Erel Segal-Halevi2013年12月15日 15:24:09 +00:00Commented Dec 15, 2013 at 15:24
-
\$\begingroup\$ 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(); }
\$\endgroup\$AJMansfield– AJMansfield2013年12月17日 02:39:24 +00:00Commented Dec 17, 2013 at 2:39
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));
}
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));
}
There are two issues in your code:
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 calculatingsize
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).Maybe more subtle: if
lines
is aList
, is may not be indexed (e.g.,lines
is aLinkedList
). Callinglines.get(i)
may then be O(i), and your whole loop be O(n2) although O(n) is doable. Using anIterator
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 yourList
.
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();
}
}
-
\$\begingroup\$ You're right, I knew I could simply join the various
String
s together with\n
as you suggest. But when writing to a file withBufferedWriter
, you're supposed to usenewLine()
instead because it's system agnostic. \$\endgroup\$asteri– asteri2013年12月14日 17:16:42 +00:00Commented Dec 14, 2013 at 17:16 -
\$\begingroup\$ Well, you can still use the above structure, replacing references to
StringBuilder
byBufferedWriter
. I edited my answer above to include the final code. \$\endgroup\$scand1sk– scand1sk2013年12月14日 21:58:31 +00:00Commented Dec 14, 2013 at 21:58
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.
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));
Use Files.write
Always consider utilizing built-in tools instead of reinventing the wheel. It makes code cleaner and less error-prone.
Since Java 7 JDK offers a one-line solution to this problem in the form of write
method featured by java.nio.file.Files
class. And JDK 8 includes an overloaded version that doesn't require specifying Charset
and uses UTF-8 for encoding.
Here's all you need:
Files.write(path, lines);
Every element of lines
list will be written to the file, with each element on a new line.
No loops, no need to instantiate and close a writer manually, the only thing required is to either handle IOException
or add a throws
clause to the method signature.
Here's how the method declaration of Files.write
looks like:
public static Path write(Path path,
Iterable<? extends CharSequence> lines,
OpenOption... options) throws IOException
The first parameter is of type Path
. It represents a location in the file system and was cleaner designed as a replacement for java.io.File
(this class is considered legacy and not encouraged to be used).
As the second parameter it expects an Iterable
containing elements of type that implements CharSequence
, i.e. String
, StringBuilder
, CharBuffer
, etc.
And lastly you can provide (if needed) as the third parameter enum constants of types LinkOption
and StandardOpenOption
in order to describe how symbolic links need to be handled and which additional actions should be performed, such as creating a new file, truncating an existing file, deleting the file after closing, etc.
If no open options were specified, it'll act as if StandardOpenOption
constants CREATE
, TRUNCATE_EXISTING
, and WRITE
were provided. I.e. a new file will be created if doesn't exist, and the file exists at the given path its contents will be truncated, and finally the file will be opened for write access.
Example
To obtain an instance of Path
you can use Path.of()
and Paths.get()
factory methods.
List<String> lines = List.of(
"The World was yang, the mountains green",
"No stain yet on the Moon was seen",
"No words were laid on stream or stone"
);
Path targetFile = Path.of("valid path in your system"); // either relative or absolute
Files.write(targetFile, lines);
The file will contain each list element on a separate line:
The World was yang, the mountains green
No stain yet on the Moon was seen
No words were laid on stream or stone
Note, that if the parent directory doesn't exist method write
will throw an exception. When that might be the case you can add the following line before the write
call:
Files.createDirectories(targetFile.getParent());
It'll create all the missing directories in the directory tree, and if everything is in place it will do nothing (no exception in such case).
-
\$\begingroup\$ Does
Files.write()
satisfy OP's requirement that the file end with an unterminated line? \$\endgroup\$Toby Speight– Toby Speight2024年04月25日 06:59:35 +00:00Commented Apr 25, 2024 at 6:59
You do not need condition check if you change the loop limit from lines.size()
to lines.size() -1
. This ensures that the last entry in the lines
is skipped. Next, after the loop, you write line
's last content.
//Write contents to the file
BufferedWriter writer = new BufferedWriter(new FileWriter(file));
int last = lines.size()-1;
for(int i = 0; i < last; i++) {
writer.write(lines.get(i));
writer.newLine(); //no condition check anymore
}
//write last line content
writer.write(lines.get(last));
First, the premise is mistaken. The file should end in a newline character to be a text file by the usual (POSIX) definition.
A file ending in a blank line has \n\n
as its last two characters.
If you really want to end in a partial line, consider writing the newline before the content, and using a flag variable to special-case the first line.
Prefer a "for-each" loop when iterating an entire collection, rather than a traditional for
loop over its indices. It's much easier for readers to see what we're doing, and much harder to make trivial mistakes such as comparing against a different variable's .size()
.
For example:
bool firstLine = true;
for (line: lines) {
if (!firstLine) {
writer.newLine();
}
writer.write(line);
firstLine = false;
}
?>
, 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. \$\endgroup\$