4 ms·

Look at some of DBell's code: http://www.tip.net.au/~dbell/ http://www.isthe.com/chongo/tech/comp/calc/index.html Learn from it. DBell is one of the

by V-2 12y ago

Look at some of DBell's code: http://www.tip.net.au/~dbell/ http://www.isthe.com/chongo/tech/comp/calc/index.html Learn from it. DBell is one of the best programmers on the planet. I have nothing against Mr DBell, whom I don't know, but... don't learn from it, please. Weather 1.9 - Java application to plot weather observations from the Australian Government's Bureau of Meteorology (BOM) web site. I did look at it. Controller.java is over 1200 lines long. Don't learn from people who create classes that big (aka God objects). SunriseSunset.java contains nearly 100 lines of commented out code. That's another bad practice. What's more, it consists mostly of a main method commented as "Simple main to test the class". This is a so-called poor man's unit test, it doesn't use assertions, it just prints some results out to be verified by the programmer manually. Which is another antipattern. His FileInfo.java (part of his FileSelection 2.0) contains tautological comments like: // // Get the suffix path. // String suffixPath = fullPath.substring(prefixPath.length()); Gee, you don't say. Redundant comments, another antipattern. isDirectoryEmpty (in the same file) happily ignores an exception: boolean isEmpty = true; try { stream = Files.newDirectoryStream(directoryPath); for (Path path : stream) { String name = path.getFileName().toString(); if (!name.equals(".") && !name.equals("..")) { isEmpty = false; break; } } stream.close(); stream = null; } catch (IOException e) { } finally { Utils.safeClose(stream); } return isEmpty; Meaning that if an IOException occurs (for whatever reason), the method will tell you that the directory is empty, even if that's not true. Setting stream to null after closing it makes no sense whatsoever (it's a local variable, it's not going to survive exiting the method). Closing it in the try block makes no sense to me either, because Utils.safeClose is going to execute either way; that's how the finally clause works. It looks like the author assumed that finally only executes if an exception happens. No, it is run in either case. So, what's the point of closing the stream twice? And if it does serve some purpose, now that would require leaving a comment, because it's totally unobvious. Not "getting the suffix path", which is blatantly obvious. I could go on. This is poor quality code, I'm hardly a great programmer (working on it!), but it's not up to my production standards. If the man is "one of the best programmers on the planet", the planet can't be Earth.

bluetomcat 12y ago
The C sources are quite baroque as well -- inconsistent naming, lots of repetition. Things that could have been neatly done with a lookup table are coded into enormous repetitive switch statements. No goto for error handling, hence a repetition of the same error handling code before each return. If you want to see examples of consistent and disciplined C programming, look at Git and Nginx.
deleted 12y ago
[deleted]
desdiv 12y ago
>So, what's the point of closing the stream twice? The first stream.close() does not get executed if an IOException occurs.
- V-2 12y ago
  Of course, but the second one DOES execute whether the exception occurs or not. So the first call is redundant. If no exception occurs (which is most of the time, probably), the code will call stream.close(); stream = null; and after that it will pass that null stream to Utils.safeClose, which can't do anything to it at this point, as the stream is closed by now and we've dropped the reference to it anyway (by setting it to null in meantime) :) The code reads as if its author was under false impression that finally only executes if catch executes. But that's not true, and it would make no logical sense (why would the language distinguish between "catch" and "finally" at all in such case?)
- jacquesm 12y ago
  The whole point of a finally block is to have the cleanup code in it that should be called regardless of whether or not an exception occurred in the first place. So the first stream.close should not be there if the 'finally' block already contains one, and it will be executed twice if an exception does not occur.

by V-2 12y ago

bluetomcat 12y ago

The C sources are quite baroque as well -- inconsistent naming, lots of repetition. Things that could have been neatly done with a lookup table are coded into enormous repetitive switch statements. No goto for error handling, hence a repetition of the same error handling code before each return. If you want to see examples of consistent and disciplined C programming, look at Git and Nginx.

deleted 12y ago

[deleted]

desdiv 12y ago

>So, what's the point of closing the stream twice? The first stream.close() does not get executed if an IOException occurs.

V-2 12y ago
Of course, but the second one DOES execute whether the exception occurs or not. So the first call is redundant. If no exception occurs (which is most of the time, probably), the code will call stream.close(); stream = null; and after that it will pass that null stream to Utils.safeClose, which can't do anything to it at this point, as the stream is closed by now and we've dropped the reference to it anyway (by setting it to null in meantime) :) The code reads as if its author was under false impression that finally only executes if catch executes. But that's not true, and it would make no logical sense (why would the language distinguish between "catch" and "finally" at all in such case?)
jacquesm 12y ago
The whole point of a finally block is to have the cleanup code in it that should be called regardless of whether or not an exception occurred in the first place. So the first stream.close should not be there if the 'finally' block already contains one, and it will be executed twice if an exception does not occur.