|
Ed, Regarding: ------------------------ As a side note, the term "helper" should be eradicated from our
dictionary. All features need to be implemented in a domain
appropriate class to optimize reusability and flexibility. Helper
classes are like constant classes, they serve as the procedural
programmers dumping ground for stuff that they can't work where to put
it. You used the term differently in that you intend methods to go in
those classes. It is just a distasteful term.
------------------------ Consider the class: org.apache.commons.lang.StringUtils It contains a bunch of static methods that operate on Strings. In other words, it is a helper class. The domain object of concern is java.lang.String. How would you provide the functionality of StringUtils without modifying the String
class? I will appreciate any suggestions. Thanks, Abdul
Edward Sumerfield <esumerfd@xxxxxxxxxxxxxx> wrote: On 10/18/06, Wehby, Mark wrote:
> I generally agree with the comments below, except for the part on
> Exceptions. I prefer unchecked exceptions over checked,
Agreed, but this is where we part ways.
> but I usually
> do not find it useful to define a hierarchy of unchecked exceptions.
> Most of the time, the calling code can figure out what to do by using
> the getCause() method on the Throwable class.
I would call this a leaky abstraction code smell.
http://en.wikipedia.org/wiki/Leaky_abstraction
http://www.joelonsoftware.com/articles/LeakyAbstractions.html
The cause of the exception is useful
for logging and debugging but if
your code is making decisions based on it then you are assuming an
implementation of the API you are using.
The API of a class exposes methods, and thrown exceptions (and public
properties but we good little programmers always use getters). This
API needs to be strictly enforced by the caller or changes in
implementation behind the API are likely to break the callers code.
For this reason, enhancing the API to tell the caller enough
information so that they can determine what happened without exposing
any implementation secrets is a more decoupled, less leaky, approach.
> Unless I can add "helper"
> methods on the derived unchecked runtime exception class, I try to use a
> RuntimeException from the JDK.
Use of any of the root exceptions if a bad idea. It's about "positive
programming". I believe there is a more common term for this approach
but I can put my finger on it.
Anyway, each line of code you write
should serve one purpose and that purpose should not accidentally
achieve side effects.
Catching Exception or RuntimeException means that you will be catching
any number of other situations that you do not intend to catch. Say
someone adds a call to Integer.parseInt lower in the stack somewhere
you will now catch that as well not knowing that it is a different
situation that you are catching. This can hide bugs and cause
inaccurate/unintended reactions to situations. Its the unintended part
that is dangerous and not "positive programming".
> I am not sure if this is a great example of a useful "helper" method,
> but say you have a web site where users must subscribe in order to read
> articles. The first time a user visits, he/she fills in the username
> and password fields and submits it for approval. On the server, there
> is code that checks to make sure the username is
unique. In the case
> where the username is not unique, the code throws a RuntimeException
> with a useful message. On the other hand, we could try to help by
> creating a custom DuplicateNameException that extends RuntimeException.
> We could add a method called "suggestedUsernames". This method could
> use some fancy algorithm to generate unique names that are similar to
> the original username.
The DuplicateNameException is good because it becomes part of your
public API, these "helper" methods are a way to allow the caller to
access information like the caused-by without becoming coupled to the
type. So you might have a method isDatabaseConnected or something that
checks for a caused by of a SQLException but doesn't expose the
SQLException type to the caller.
Now the suggestedUsernames is interesting because it exists in a
domain type that has nothing to do with usernames or any kind of
fancy
algorithm class. I would argue that the exception class should serve
as a simple notification class, only supplying additional information
about the situation. Then another domain class would be used to
determine suggested names. FancyUserNameSuggestor or something.
In the past I have used specific RuntimeExceptions as stack
notifications. Imagine a web server that receives requests through a
common handler. Processing continues down the stack until it finds an
error that the common handler needs to react to. You could either
choose to return some common Result instance back up the stack or
throw an exception that you know the common handler is going to catch
and handle.
This becomes a sort of notification mechanism allowing deep methods to
talk to shallow methods. We only used it in two situations and it was
very useful. We made the rule that only web layer code could perform
the throws, no business or DAO layer
should couple themselves into a
"stack" based communication mechanism. After all, they might be moved
to an ejb container somewhere one day and we wouldn't want any stack
coupling.
As a side note, the term "helper" should be eradicated from our
dictionary. All features need to be implemented in a domain
appropriate class to optimize reusability and flexibility. Helper
classes are like constant classes, they serve as the procedural
programmers dumping ground for stuff that they can't work where to put
it. You used the term differently in that you intend methods to go in
those classes. It is just a distasteful term.
> Just my $0.02
> Mark Wehby
>
> -----Original Message-----
> From: esumerfd@xxxxxxxxx [mailto:esumerfd@xxxxxxxxx] On Behalf Of Edward
> Sumerfield
> Sent: Tuesday, October 17, 2006 4:39 PM
> To: users@xxxxxxxxxx
> Subject: [cinjug-users] Brandan Jones
Presentation
>
> 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
>
> ---------
> You may unsubscribe from this mailing list
> by sending a blank email addressed to:
> users-unsubscribe@xxxxxxxxxx
>
> --
> Find additional help by sending a blank email
> addressed to:
> users-help@xxxxxxxxxx
>
>
> ---------
> You may unsubscribe from this mailing list
> by sending a blank email addressed to:
> users-unsubscribe@xxxxxxxxxx
>
> --
> Find additional help by sending a blank email
>
addressed to:
> users-help@xxxxxxxxxx
>
>
--
Ed
---------
You may unsubscribe from this mailing list
by sending a blank email addressed to:
users-unsubscribe@xxxxxxxxxx
--
Find additional help by sending a blank email
addressed to:
users-help@xxxxxxxxxx
Yahoo! Messenger with Voice. Make PC-to-Phone Calls to the US (and 30+ countries) for 2¢/min or less.
|