Skip to main content
Code Review

Return to Answer

Javadoc and Unit tests
Source Link
Ralf Kleberhoff
Ralf Kleberhoff
  • 3.3k
  • 10
  • 12

Javadoc

You are creating a public API to be widely used. Such an API should be absolutely clear to any potential user (including you, a few months later). It's a good idea to document all public aspects following the Javadoc conventions. While the main methods nearly speak for themselves, a valid use case for the update() method (found in the comments) isn't self-explanatory. Anyway, public API should generally get Javadoc. It doesn't take much time right now, but it will typically save you from headaches later.

Unit Tests

Another thing that you should do is write JUnit tests to check the Observer functionality.

This way you get two benefits:

  • Quality assurance that your code does what it's supposed to do,
  • Documentation of typical usage patterns, how to set up and call the observer/listener system, and what to expect as results.

You could even switch to Test Driven Development that

  • starts with the unit tests that once your code should pass (all tests fail or don't even compile),
  • then write and edit the implementing code until it passes the tests.

Javadoc

You are creating a public API to be widely used. Such an API should be absolutely clear to any potential user (including you, a few months later). It's a good idea to document all public aspects following the Javadoc conventions. While the main methods nearly speak for themselves, a valid use case for the update() method (found in the comments) isn't self-explanatory. Anyway, public API should generally get Javadoc. It doesn't take much time right now, but it will typically save you from headaches later.

Unit Tests

Another thing that you should do is write JUnit tests to check the Observer functionality.

This way you get two benefits:

  • Quality assurance that your code does what it's supposed to do,
  • Documentation of typical usage patterns, how to set up and call the observer/listener system, and what to expect as results.

You could even switch to Test Driven Development that

  • starts with the unit tests that once your code should pass (all tests fail or don't even compile),
  • then write and edit the implementing code until it passes the tests.
Source Link
Ralf Kleberhoff
Ralf Kleberhoff
  • 3.3k
  • 10
  • 12

API

Besides the obsever management (add/remove one), an Observable should only expose a get() and a set() method. Calling the update() method from outside should never be necessary, it should only be done from inside. And there should be no way to manipulate the item from outside this class.

I'd recommend to make the item field and the update() method private.

The Observer API (in the update() method) only gets the information about the new value. This is a working minimum. But you might consider providing additional information:

  • The Observable that got updated. This would allow one observer to monitor more than one Observable.
  • The previous value. This would allow some more elaborate reaction schemes, based on comparing the old and new value.

Of course, adding these parameters to the call doesn't come for free, so it's not clear whether this will be beneficial to you or not.

Loophole

What typical Observer approaches like yours can't get is changes within the item. E.g. an Observable<List> can't see when someone adds an element to the list. There's no easy solution to that, so you should be aware of the pitfall.

Of course, you can create an ObservableList class that informs its observers about alterations, but then there are sets, maps, arrays and so on. You'd end up with a huge zoo of specific observable classes.

Naming

The method pair observe() and removeObserver() should be named along the same pattern, making it clear that they complement one another. A typical Java naming would be addObserver() and removeObserver(). But observe() and unobserve() might be valid alternatives, if you prefer.

It's convention to name type parameters with single, upper-case letters. So, your Item type parameter should be renamed T (or I, if you prefer). To me, seeing an identifier written Item immediately implied that it's a real class or interface. It took my some time to recognize that in your case, it's a type parameter.

I'd rename the Observer.update() method to something like Observer.valueUpdated(). Calling a method named update() sounds like a command to update the item, but here the observer gets informed that something has been updated.

Inconsistent example code

Although Observable.set() already includes the necessary update() call, in e.g. Box.setBoxName() you still have a redundant call:

public void setBoxName(String str) {
    boxName.set(str);
    boxName.update();
}