Builder for a simple observer class

Question

I mean the Builder presented in Effective Java by Joshua Bloch.

OnStopScrollingObserverTask is using to detect when scrolling a ScrollView stops.

Please criticize my code and tell me if using Builder for such a simple class is too excessive.

public class OnStopScrollingObserverTask implements Runnable {
 public static interface Listener {
 void onScrollStopped();
 }
 public static Builder builder() {
 // Use this static method instead of Builder's constructor
 // in order not to write the keyword << new >> every time
 return new Builder();
 }
 public static class Builder {
 private Listener listener;
 private Long delay;
 private Helper helper;
 private View view;
 private Builder() {
 // empty constructor
 }
 public Builder delay(Long delay) {
 this.delay = delay;
 return this;
 }
 public Builder listener(Listener listener) {
 this.listener = listener;
 return this;
 }
 public Builder helper(Helper helper) {
 this.helper= helper;
 return this;
 }
 public Builder view(View view) {
 this.view = view;
 return this;
 }
 public OnStopScrollingObserverTask build() {
 checkPredicates();
 OnStopScrollingObserverTask task = new OnStopScrollingObserverTask(view, helper);
 task.setListener(listener);
 task.setDelay(delay);
 return task;
 }
 private void checkPredicates() {
 checkRequiredArgumentsAreNotNull();
 }
 private void checkRequiredArgumentsAreNotNull() {
 for (Object arg : requiredArguments()) {
 if (arg == null) {
 throw new IllegalStateException("Some of required arguments are not defined");
 }
 }
 }
 private Object[] requiredArguments() {
 return new Object[] { view, helper };
 }
 }
 private static final long DEFAULT_DELAY = 100;
 private static final Listener DUMMY_LISTENER = new Listener() {
 @Override
 public void onScrollStopped() {
 // dummy implementation
 }
 };
 private final Helper helper;
 private final View view;
 private int initialPosition;
 private Listener listener;
 private long delay;
 private OnStopScrollingObserverTask(View view, Helper helper) {
 this.view = view;
 this.helper = helper;
 this.listener = DUMMY_LISTENER;
 this.initialPosition = getScrollCoordinate();
 }
 private int getScrollCoordinate() {
 return helper.getScrollCoordinate(view);
 }
 public void setDelay(Long newDelay) {
 this.delay = (newDelay != null) ? newDelay : DEFAULT_DELAY;
 }
 public void setListener(Listener newListener) {
 this.listener = (newListener != null) ? newListener : DUMMY_LISTENER;
 }
 @Override
 public void run() {
 int newPosition = getScrollCoordinate();
 int delta = initialPosition - newPosition;
 if (delta == 0) {
 listener.onScrollStopped();
 } else {
 this.initialPosition = getScrollCoordinate();
 view.postDelayed(this, delay);
 }
 }
 public void startDelayed() {
 this.initialPosition = getScrollCoordinate();
 view.postDelayed(this, delay);
 }
}

Usage:

if (motionEvent.getAction() == MotionEvent.ACTION_UP) {
 OnStopScrollingObserverTask observer = OnStopScrollingObserverTask.builder()
 .helper(helper)
 .view(scrollView)
 .listener(new OnStopScrollingObserverTask.Listener() {
 public void onScrollStopped() {
 if (ViewUtils.isScrollViewScrolledToTheEdge(scrollView)) {
 ScrollViewEffects.showEdgeEffectOnScrollStopped(scrollView);
 }
 }
 }).build();
 observer.startDelayed();
}

• jsc0: The builder pattern is used when a class uses a variable number of arguments, right? It stops you from creating a ton of constructors in your class. It doesn't seem like this class really needs to use that pattern.
• Leonid Semyonov: @jsc0 but there is delay and listener are optional arguments.

Answer 1

The Builder pattern is most useful when a class needs too many (4+) parameters to configure correctly, some of which are optional or can have reasonable defaults. Your case looks a bit different. The OnStopScrollingObserverTask class has a single constructor with 2 arguments.

---

You may have noticed that checkRequiredArgumentsAreNotNull is not very elegant. If you look at all the methods that get called from that point, it's a whole pageful of code that's neither elegant nor very useful.

What if you make a mistake with the builder and forget to set one of the required arguments, the view or the helper? Your code will compile, it may seem to be fine, but at runtime you'll get a IllegalStateException. This is a very fragile design. You really want to detect this kind of errors at compile time.

I recommend a slight deviation from the standard Builder pattern: instead of taking no arguments, make it take the absolutely essential arguments:

public static class Builder {
 private final Helper helper;
 private final View view;
 private Builder(Helper helper, View view) {
 this.helper = helper;
 this.view = view;
 }

This way the class will always have the required elements, and you will catch any misconfiguration as soon as possible, at compile time.

---

On closer look, your implementation can be changed to match the builder pattern better. Consider for example this constructor:

private OnStopScrollingObserverTask(View view, Helper helper) {
 this.view = view;
 this.helper = helper;
 this.listener = DUMMY_LISTENER;
 this.initialPosition = getScrollCoordinate();
}

And the builder that uses the constructor:

public OnStopScrollingObserverTask build() {
 checkPredicates();
 OnStopScrollingObserverTask task = new OnStopScrollingObserverTask(view, helper);
 task.setListener(listener);
 task.setDelay(delay);
 return task;
}

As it is, this kinda goes against the builder pattern. The builder should not have to call setters after creating the object. Consider this alternative:

1. Eliminate these setters
2. Make listener and delay immutable
3. Let the builder pass itself to the constructor, and let OnStopScrollingObserverTask get the fields from the received builder
4. Instead of setting the default listener and delay in OnStopScrollingObserverTask, do it in the builder

Suggested implementation

Applying the above changes, this is more robust and elegant:

public static class Builder {
 private final Helper helper;
 private final View view;
 private Listener listener = DUMMY_LISTENER;
 private Long delay = DEFAULT_DELAY;
 private Builder(Helper helper, View view) {
 this.helper = helper;
 this.view = view;
 }
 public Builder delay(Long delay) {
 this.delay = delay;
 return this;
 }
 public Builder listener(Listener listener) {
 this.listener = listener;
 return this;
 }
 public OnStopScrollingObserverTask build() {
 return new OnStopScrollingObserverTask(this);
 }
}
private final Helper helper;
private final View view;
private final Listener listener;
private final long delay;
private int initialPosition;
private OnStopScrollingObserverTask(Builder builder) {
 this.view = builder.view;
 this.helper = builder.helper;
 this.listener = builder.listener;
 this.delay = builder.delay;
 this.initialPosition = getScrollCoordinate();
}

Important note: be careful, copying mutable reference types is not safe. If view, helper or listener are mutable, this is not safe. For example, if the caller of the builder mutates the view, it will be changed in OnStopScrollingObserverTask too. Another problem is if the caller reuses the builder to create more OnStopScrollingObserverTask instances, they will end up sharing the same view and helper. So try to use defensive copies for these.

• Leonid Semyonov: or just to pass the builder in OnStopScrollingObserverTask constructor and copy builder's fields in there
• janos: Excellent idea, and that's how it is in the book too. I updated my answer accordingly, thanks for pointing out!