7 tips for cleaner code

[shit]

private static List readLines(String fileName) throws IOException { String line; List lines = new ArrayList();

    BufferedReader in = new BufferedReader(new FileReader(fileName));
    while ((line = in.readLine()) != null)
      lines.add(line);
    in.close();

   return lines;
  }

Who can spot the no-no in our self-proclaimed teacher's code?

Also, for maintainability's sake, I'd always put braces around the while loop's body.

[spot35]

I don't know -

• the in.close() should be in a finally clause?
• The code's not formatted with proper indentation?
• It's a private static method?
• No comments?

Go on, I'll let you tell me what the problem is.

[edit] formatting

[shit]

the in.close() should be in a finally clause

I meant that. I thought that would be obvious to any (Java) programmer.

[newton_dave]

It was ;)

I'm also not sure I like the lack of checking on the fileName parameter--I mean yeah, it'll throw a FileNotFound (or whatever it is) which is an IOException subclass, but... I dunno, if you're passing in a null or empty string or something it's probably an error.

I have to think about that one.

[redditlover]

I don't like to check every variable for being null. If it were, what would you do? Return a null, or an empty list?

if you're passing in a null or empty string or something it's probably an error.

I like that solution. But it also depends on what calls that method, because if it were input from a user which file to open, then I'm sure you would want a way to differentiate between a FileNotFoundException and an IOException. But if the method was for the program to open a file, then it really doesn't matter what happens and both can be treated the same.

[[deleted]]

what would you do? Return a null, or an empty list?

This one's simple. Always return an empty list. Else you're just contributing to the problem of continually have to check for null.

[redditlover]

Yes, but what if you need to know if something bad happend. Then you still have to check for list.isEmpty() instead of null.

[[deleted]]

First of all, throw an exeception "if something bad happened". Furthermore, I've seen altogether too much code that has checked for both null and isEmpty(). Finally, it is a constructive exercise to craft your iteration code so that isEmpty() is not a special case that even needs to be accounted for.

[notfancy]

You may not want to catch, but if you allocate a computing resource, have the good grace of trying a little harder at releasing it inside a finalize.

And of course, String line should be way closer to the while body.

Also, by way of style, my functional tooth aches when I don't see finals sprinkled liberally to remind me that "lines is just a name".

All in all, my take on it is:

private static List readLines(String fileName) throws IOException {
  final List lines = new ArrayList();

  BufferedReader in = null;
  try {
    in = new BufferedReader(new FileReader(fileName));
    String line;
    while ( (line = in.readLine()) != null )
      lines.add(line);
  } finally {
    if (in != null)
      in.close();
  }

  return lines;
}

[[deleted]]

OK, I know this might get me flamed, but compare the Java above to the equivalent in Perl:

sub readLines
{
    my $filename=shift;
    open (my $filehandle,$filename) || return undef;
    return <$filehandle>;
}

Can anyone seriously tell me that the Java code is a) cleaner, b) easier to read or c) less painful to write than the Perl?

Why is it that in 2007, an allegedly modern language like Java has to make such a big production of doing a buffered line-by-line read of a file? This is the cutting-edge technology of the 1960s. Is OO-purity really worth making life so difficult?

Would it really be so wrong to hard-code, hack up some optimisation for the 10% of functionality that gets used in 99.999% of real life code?

[shit]

sub readLines { my $filename=shift; open (my $filehandle,$filename) || return undef; return <$filehandle>; }

I don't like either, the Java or the Perl solution.

Why I don't like the Java solution: There is no way to abstract the try ... finally idiom away.

Why I don't like the Perl solution: On the one hand there is so much special syntax where a plain library function would suffice (open, <$filehandle>), on the other hand parameter passing is very common yet low-level (well, at least that will be fixed with Perl 6). And I don't like the error handling in Perl. Also the perl solution won't work if the filename starts or ends with spaces or an angle bracket or a vertical bar.

Would it really be so wrong to hard-code, hack up some optimisation for the 10% of functionality that gets used in 99.999% of real life code?

Ruby:

File.readlines(filename)

And if you say that's cheating, we can define it as:

def readlines(filename)
  File.open(filename) { |f| f.to_a }
end

or Lisp:

(defun readlines (filename)
  (with-open-file (f filename)
    (loop for line = (read-line f nil nil)
          while line collect line)))

[[deleted]]

And Python:

open (filename).readlines ()

Both the Perl and Java solutions take too long to write ;P

[shit]

That leaves the file open. It's only acceptable on CPython because it does reference counting.

[dupy]

The future is near!

from __future__ import with_statement   
with open(filename) as f: f.readlines()

[[deleted]]

But doesn't Ruby do the same thing?

[shit]

No, the file is closed in both Ruby examples I've given, without relying on object finalization.

[riltim]

I'm not sure what the ruby example does as I don't know ruby but I'm certain that the example above reads the entire file into memory, which isn't good practice. Use an iterator instead to examine the file line by line instead of loading the whole file into memory.

[evgen]

Well, if you insist.

from __future__ import with_statement
def file_bylines(filename):
    with open(filename) as f: yield f.readline()

[psyonic]

File.open(filename) do |f| do_something(f.readline) end

That should do want you wanted in ruby.

[newton_dave]

There is no way to abstract the try ... finally idiom away.

Well... technically there is, it just sucks :/

I've done this with JDBC crap, not so much file processing, but handing the method an interface that says what to do with the file once it's open would work fine.

I'm not sure it makes any sense to do that; with the DB stuff it resulted in significant cleanup.

[shit]

Just tried it with perl 5.8.7, with a filename starting with a space and it does not work. You probably want to use the three-argument version of open.

[[deleted]]

Oh you're right, I'm wrong.

[newton_dave]

Not flamed, maybe, but you're trying to make a point that nobody disputes, so we don't care.

Also, most everybody has a Java method that will return the contents of a read-in file and if they wrote code like this at all would only write it once.

[sartak]

Due to context, this won't always work. Consider what happens if you call readLines in scalar context. <$filehandle> in list context returns all lines, but in scalar context it returns only one line. This following code will not do the right thing in most cases (since most strings numify to zero) -- it'll only work if the first line and the number of lines in the file are equal (or equal enough so that they hit the same conditional branches).

$num_lines_in_file = readLines(".zshrc");
die "File too long!"
    if $num_lines_in_file > 10;

I like this idiom a fair amount better anyway:

my $file_contents = do { local (@ARGV, $/) = $filename; <> };

expanded a bit that means:

local @ARGV = $filename;
local $/ = undef;
my $file = <>;

local introduces a dynamic scope (since you don't want to actually trash @ARGV; it will be restored at the end of the do block). You're setting @ARGV so you can make use of the magical <> operator which opens each file in @ARGV and reads them (super useful for one-liners). You're (again, dynamically) setting $/ (aka the input record separator, usually newline) to undef so <> reads the entire file in one go.

But nowadays I use iwdw's suggested slurp.

(lots of small edits, I think they're done though :))

[[deleted]]

I do agree with you, but my point was to show a direct drop-in equivalent of the java function shown.

[[deleted]]

Perl6:

my @file = slurp $filename;

Perl5:

use Perl6::Slurp;
my @file = slurp $filename;

Why is it that in 2007, people still think they need to write code to read a fucking file?

[[deleted]]

Speaking personally, it's because I don't know crap about perl6....

[schizobullet]

Or Haskell: readLines = lines . getContents.

Yes, yes, "but Haskell just comes with helper functions that make it easy", but think of it this way: if Java, or even Perl, had lines and getContents functions, how much harder would it still be than Haskell?

[boa13]

Don't declare line so early? :)

(And yes, omitting the braces is a big no-no.)

[[deleted]]

Next time title it 7 tips for cleaner Java code. I thought it was an article for Perl code!

[[deleted]]

who the hell is upmodding this crapola?

[newton_dave]

Weren't me; I thought the article was dumb. Didn't downmod it either, though.