4

Consider a batch operation that may or may not take long time to do its job (depends on data). A user can register an OPTIONAL listener for tracking job progress.

NOTE: listener registration is totally optional and user may want to call the job without registering any listener.

Q: Which of the following solutions are your preference and why?

EDIT: the concern here is performance vs clean code. some say, checking null reference (solution 1) is faster compare to second solution. but second solution is cleaner and more understandable. I would like to have your opinion.

No 1: Allow null listener and always check if listener is not null, then call it.

doMyBatchJob() {
   if (listener != null) {
      listenr.progressStarted(params);
   }
   while (x) {
      if (listener != null) {
          listener.progressUpdated(current, expected)
      }
   }
   if (listener != null) {
      listenr.progressFinished(params);
   }
}

No 2: Implement a dummy listener, and register it if user didn't pass his/her own listener. so that can call listener without checking for null object.

DummyListener {
     public void progressStarted(params) { //DO NOTHING }
     public void progressUpdated(current, expected) { //DO NOTHING }
     public void progressFinished(params) { //DO NOTHING }
}

doMyBatchJob() {
   listener.progressStarted(params);
   while (x) {
         //Do a single unit of the batch operation
         // ... code omitted here
         listener.progressUpdated(current, expected)
   }
   listener.progressFinished(params)
}
mhshams
  • 16,384
  • 17
  • 55
  • 65
  • 2
    what you want is the `Null Object Pattern` –  Nov 19 '12 at 03:04
  • Perhaps [this](http://stackoverflow.com/questions/5981145/alternatives-to-returning-null) is what you're asking? – mre Nov 19 '12 at 03:14

4 Answers4

4

You are correct in your concern that if x==null is a code smell, it definitiely is!

There are very valid reasons for using the Null Object pattern, one is to keep from littering your code with if (x == null) noise that is usually less business related and more poor design related. NULL means an absence of a value not a default value.

I don't think you are taking the Null Object pattern far enough.

Never return null and you never have to check for it

First off never return null from a method and never have if x == null in your code. Both are sure signs of poor design. null references and NPE should be an error that should be resolved where it doesn't happen.

Never accept null and you never have to check for it.

Have methods that return null return a Null Object and have things that accept null and might process null references have Null Object implemenations to process the Null References.

In your case your Dummy object would not just not do anything, it should report to the log warnings that it has encountered a null and something should be done about it.

Use a good JSR305 library to annotate your methods so that they dont' accept nulls.

I have the com.google.code.findbugs maven dependency in every Java program I create, no matter how trivial, then I can decorate every method and method parameter with @NONNULL and not have to worry about writing if x == null ever again!

If you have 3rd party code that returns null wrap it and use the JSR305 annotations.

Use the Guava Preconditions.checkNotNull() with a static import so you can do checkNotNull() on all parameters that are marked @Nonnnull, you can even include a descriptive error message about what was, null and why it should not be or whatever.

And smugly think about how poorly designed their code is.

Community
  • 1
  • 1
  • Thanks for the answer, really appreciate it.The thing is that some of my clients are not really interested in passing the listener, they don't need it for few reasons so I can't force them to pass the listener. – mhshams Nov 19 '12 at 15:10
  • @mohammadshamsi then don't make them, add a default "NullListener" as the "default" listener, and if they don't pass one then it will be used. have one method signature that requires it, and another that doesn't that provides the `Null Object` listener that doesn't do anything ( or does some default behavior ) –  Nov 19 '12 at 22:07
  • then you suggesting the solution #2. – mhshams Nov 20 '12 at 03:01
  • That's very black&white. Yes, "Null Object" makes perfect sense here. But I don't think saying **never** to `null` is very wisely. "Null behaviour" greatly depends on context. With collections or listeners it's pretty much the same all the time. But it might not be the case for other objects. –  Nov 20 '12 at 09:31
  • @pavelrappo Tony Hoare ( the inventor of the null reference ) thinks it is a terrible design decision and called them a ["Billion Dollar Mistake"](http://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare), who I am I argue with him. There are languages where there is no concept of `null` ( or any equivalent ) and they don't suffer from this manufactured *design decision* ( problem ) and are less buggy by far. –  Nov 20 '12 at 13:11
  • @JarrodRoberson I think you can argue with someone until you have reasonable arguments. But that's me :) I really don't think the problem is the `null` itself -- it's the semantics of `null` in each place. If you feel some function shouldn't accept or return `null` -- that's fine. Annotate it, check it in runtime, mention it in javadoc... whatever. Deal with it as with any other illegal value. Sometimes it's just the marker or special value. Change it to anything you want and you still have to worry about the checks: `node.getParent() != Node.NONE` or `node.hasParent()`. –  Nov 20 '12 at 16:30
  • 2
    @pavelrappo *"sometimes it is a marker or special value"* this is the problem, the semantics of `null` is very well defined, it is the **absence of data** it has a special meaning that is well defined, if **you** redefine it you are causing headaches for everyone else. I can't tell you how much code I have come across in the last 25 years that has crap like `if x == null then x = 2` with no explanations and where `x = 14` in other places for no apparent reason. The creator says it was a mistake and decades of use have proven it. Avoid `null` in your designs so others don't pay for it. –  Nov 20 '12 at 18:57
1

I would model the batch job as an Observable and the listener as an Observer. The state changes within the batch job can be tracked in an object that is used to communicate with Observers through the notifyObservers(object) method.

Vikdor
  • 23,934
  • 10
  • 61
  • 84
  • the question is about having a null reference or not. even if you implement Observable pattern, the question is still valid. – mhshams Nov 19 '12 at 04:42
  • When you implement Observer pattern, if the user doesn't provide a listener, no observers are added to the Observable batch job. But the Observable will still continue to call notifyObservers except that it will be a no-op. As an observable, the batch job doesn't need to check if there are any listeners or not and that would address your question, right? – Vikdor Nov 19 '12 at 04:44
  • As you mentioned, Observable will still continue to call notifyObservers. My question is whether it's worth to check if we need to call this method or not ? (if no observer registered, calling notifyObservers is not necessary) – mhshams Nov 19 '12 at 04:48
  • That's the pub-sub paradigm where the publisher is not aware of who the subscribers and need not be as it is decoupled. It just publishes the updates. If there are any subscribers they consume it, otherwise it's a no-op. – Vikdor Nov 19 '12 at 04:55
-1

Answer depends on the real intention behind the default listener. If your default listener(Dummy) doesn't do anything thing, then there is no need of creating such listener. Null with null-check is meant to handle such scenarios only(more efficient and compact).

But on the other hand, if you want some default behavior implemented through default listener, then you should be creating the default listener.

As a thumb rule, I would not crowd the program with objects until there is any use/requirement is there.

Yogendra Singh
  • 33,927
  • 6
  • 63
  • 73
  • the Dummy listener is not doing anything. its there just to avoid so many if statement in the doMyJob method. – mhshams Nov 19 '12 at 05:01
  • @mohammadshamsi: If no specific purpose, then I would not advice to create it(Dummy Listener). Putting `null` check is any way better and helpful as it will avoid failures if your Dummy also gets null for some reason. – Yogendra Singh Nov 19 '12 at 05:05
  • @downvoter: Please leave some helpful comments to understand the issue. – Yogendra Singh Nov 19 '12 at 13:47
-1

I would do neither. I would use option 1, but greatly simplified:

doMyBatchJob() {
   if (listener == null) {
       return; // if listener is null, do nothing
   }
   listener.progressStarted(params);
   while (x) {
      listener.progressUpdated(current, expected)
   }
   listener.progressFinished(params);
}

This follows the "exit early" mantra. Also, it does't result in "class bloat" - your DummyListener idea adds no value. Using a null instead is clean and clear - everyone knows what's going on if the listener is null (ie nothing) and it's more obvious when debugging.

Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • please read the question again. we have a job to do within this method. – mhshams Nov 19 '12 at 04:38
  • @mohammadshamsi I read the question again and can not find anything to indicate that this is not a good answer. It is logically identical to his option 1, but easier to read and will perform better than either of his options, thus answer is the best way to do it. Could you please tell me exactly why thus answer is "not helpful", which is what a down vote is used for – Bohemian Nov 19 '12 at 15:55
  • within while loop you can see the comment about omitted code "//Do a single unit of the batch operation". we have something to do in the while loop regardless of the absence of listener. So we can't simply stop the job, just because listener is not there. (sorry about down vote, it wasn't me) – mhshams Nov 21 '12 at 02:57