A great presentation by Brandon last night. As always, I learned
something new. CinJug rocks.
It sounded like there were some java newbies there so I thought it
would be interesting to pick apart the parts of Brandon's code that
were bad. I hope all the newbies are on the list and that no one takes
"newbie" in any derogatory sense.
We all understand that Brandon's presentation was focused on Patterns
so this is no reflection on his code since it served its purpose well.
Instead it may serve as a fun extension to that code to look at what
is good and bad about what we do each day.
If you don't recognize my name and were at the meeting you will know
me by "I am the runner, execute".
Exceptions
----------------
The exceptions were used in the file io process and it brings up lots
of good practice issues.
try {
Scanner scanner = new Scanner(file);
...
}
catch (FileNotFoundException e)
{
throw new RuntimeException(e);
}
1) Should we every throw a RuntimeException?
Your applications should never throw or catch Exception or
RuntimeException. Instead, your application should use a well defined
exception hierarchy that allows exceptions to be addressed at
appropriate levels.
In this case we should throw something like
FileNotFoundRuntimeException. There might also be an
InvalidFileFormatRuntimeException that can be thrown if the scanner
doesn't find what it expects. This is text based UML, where ^ means
extends.
RuntimeException
^
FileImportFailedRuntimeException
^ ^
InvalidFileFormatRuntimeException FileNotFoundRuntimeException
With this structure the thrower can now throw an exception that is
specific to the problem. The catcher can catch the specific or the
higher level exception depending on how they want to manage the
problem.
2) Should we use checked or unchecked exceptions?
There is a great deal of debate in the industry about which are the
better approach. You will tend to find that people that like strongly
typed languages also like checked exceptions, where as those who
prefer dynamically typed languages tend toward the unchecked variety.
I will give you my perspective and let others posture the alternatives.
Checked exceptions force the user of an API to deal with the problems
that the API might cause. At first sounding this seems good.
Unfortunately, in reality the "force" portion of this process is
thrust upon the developer before they really know what the program
should do about it. The result is that code gets a lot of empty catch
blocks or System.out.println's.
The hardest bugs to find are those that don't leave any evidence and
empty catch blocks are the worst. Even works that null pointer
exceptions but we will get to those next.
I am a proponent of forcing real errors to the surface as fast as
possible which means crashing the program in front of the developer.
To do this I propose only using RuntimeExceptions and allowing the
developer to choose the best time and place to insert the catch
statement.
When the developer has got to the point of running the program and
finding the error situation they are at the right point to know what
to do about it and can insert the catch of the runtime exception and
the appropriate behaviour.
The other advantage of the RuntimeException is that you don't have to
declare it on all the methods that might end up calling a thrower.
This loosens the coupling between your APIs and reduces the number of
files that change when you change you mind about the exceptions to
throw.
Nulls
-------
One of the early versions of Brandon's factory method went something like this:
if (param.equals("something")) {
return new Something();
}
else if (param.equals("anotherthis")) {
return new AnotherThing();
}
return null;
Nulls are the death of a system. Traditionally developed software
follows a few predictable phases, code, test, fix bugs, release to
production, fix all the null pointer exceptions.
These bugs are hard to find because the exception report doesn't tell
you where the error occurred, only where the null value was eventually
used.
The most common fix is "if (x != null)". This is not the right answer
but sometimes we are forced into bad code by bad 3rd party libraries.
What can I say, most of them suck. Yes, I am opinionated, and they
still suck.
The correct answer is to not produce nulls. If no code ever returns a
null then you will never get a NullPointerException.
Brandon's final version converted over to throwing an exception (not
the correct type but we have talked of that already) so the factory
can not produce a null.
Take this to the next level. A method getList(). What should it do if
there is no list available. It should return an empty array list. Sun
was kind enough to supply is with a simple version in
Collections.EMPTY_LIST or emptyList in 1.5.
In all cases there are ways to design your APIs to prevent nulls from
being produced. Take the extra time to do this because you will earn
it back in droves as you sit back and watch your not so well educated
colleagues shouting about a bug that "just can't be happening".
Single Return Statements
-------------------------------------
This one is slightly contentious as well. A method should only have
one return statement in it. Brandon's original factory method had 4.
This becomes a real problem in longer methods when you can't always
tell if the code nearer the bottom will get executed.
Ideally all you methods should be short as well but these features
play against each other as a nice piece of code gets hacked on by 20
different developers over a few years.
Some argue that in short methods it doesn't matter. I argue that short
methods become long methods so it always matters. I never use more
than one return statement even on the simplest cases.
MonthlyCommand command;
if (param.equals("something")) {
command = new Something();
}
else if (param.equals("anotherthis")) {
command = new AnotherThing();
}
else {
throw new InvalidFileTypeRuntimeException("Ooops");
}
return command;
Brandon's final reflective approach got rid of this problem anyway and
you will find that good design principles will tend to get rid of
these bad coding habits auto magically. It's all about the the strong
karma that you develop with each excellent piece of code.
DRY
------
Don't Repeat Yourself. We all know that we shouldn't duplicate code.
Copy Paste is a bugs best friend, the easiest way to spread through a
system. Brandon's code ended up with a set of MonthlyCommand
implementations that included a try catch for FileNotFoundException, a
scanner and a set of file specific fields.
public void execute(file) {
try {
Scanner scanner = new Scanner(file);
String name = scanner.next();
String amount = scanner.nextInt();
System.out.println(name + " " + amount);
}
catch (FileNotFoundException e)
{
throw new NoInputFileRuntimeException("Could not find
input file", e);
}
}
So how DRY is this code. There are four lines of duplicate code here.
Lets try a simple pull up refactoring to get rid of them:
First in the abstract base class.
public void execute(file) {
try {
Scanner scanner = new Scanner(file);
parseFields(scanner);
}
catch (FileNotFoundException e)
{
throw new NoInputFileRuntimeException("Could not find
"+file.getName(), e);
}
}
protected abstract void parseFields();
Then in the original MonthlyCommand implementation that is not
extending our abstract base class.
public void parseFields(Scanner scanner)
{
String name = scanner.next();
String amount = scanner.nextInt();
System.out.println(name + " " + amount);
}
It's a very simple change, it centralizes the error handling which is
really important and cuts down the amount you have to type in for each
command you write. You know there is a limit to the number of keys you
can press in a lifetime, use them wisely.
Open Close Principle
-------------------------------
Brandon talked about using these patterns to reduce the number of
classes that you have to edit to add new functionality. This is called
the Open Close Principle. Once you live by this you will significantly
improve the quality of the API's that you design.
Code should be open for extension
and closed for modification.
http://en.wikipedia.org/wiki/Open/closed_principle
Unit Tests
--------------
Brandon mentioned his commitment to unit testing at the end of the
presentation. I hope everyone took this to heart. Test first
development (also called Test Driven Development) is the most
significant advancement in software engineering practice since OO.
Every class/method you write must have a corresponding unit test.
http://en.wikipedia.org/wiki/Test_driven_development
I know there are a few Universities starting to teach TDD but it is by
no means mainstream yet. Most professionals already working today do
not have an understanding of TDD. We still live in a world of "let the
QA group do the testing for me".
If I interview someone that doesn't know or use TDD, I will not hire
them. It's that simple.
Perhaps Brandon can comment on how he addresses this subject.
Thanks again for the presentation Brandon.
--
Ed
|