3
\$\begingroup\$

I am working on a personal project in university where I am currently trying to make my own implementation of a generic observer pattern that I can use throughout my project.

I would appreciate any feedback, but I am specifically interested in how to generalize it further or perhaps some change that makes it even easier to use/reuse. Basically, where would this implementation break down in the future?

Below you can see my very simple test-class which works as it should. When the name of the box is updated (either through some change to the model, or through a user update from the controller) the name in the controller is also updated.

public class ObservableTest {

    public static void main(String[] args) {
        Box box = new Box();
        BoxController boxController = new BoxController(box);

        box.setBoxName("test1");
        System.out.println(box.getBoxName());
        System.out.println(boxController.boxName);

        box.setBoxNameAlt("test2");
        System.out.println(box.getBoxName());
        System.out.println(boxController.boxName);

        boxController.changeName("test3");
        System.out.println(box.getBoxName());
        System.out.println(boxController.boxName);

    }
}

The box has a name, which is made into an observable by wrapping it in my Observable class. Whenever the observable is edited the update method should be called on the Observable.

public class Box {
    public Observable<String> boxName = new Observable<>("hello");

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

    public String getBoxName() {
        return boxName.get();
    }
}

Here is my actual Observable class, which I think is self explanatory.

public class Observable<Item> {

    public Item item;
    private final ArrayList<Observer<Item>> observers = new ArrayList<>();

    public Observable(Item item) {
        this.item = item;
    }

    public void update() {
        for (Observer<Item> o : observers) {
            o.update(item);
        }
    }

    public void set(Item item) {
        this.item = item;
        update();
    }

    public Item get() {
        return item;
    }

    public void observe(Observer<Item> observer) {
        observers.add(observer);
    }

    public void removeObserver(Observer<Item> observer) {
        observers.remove(observer);
    }
}

Here is the Observer interface.

public interface Observer<Item> {
    void update(Item item);
}

And finally the controller of the box which is listening for updates of the boxes name.

public class BoxController {

    private final Observable<String> boxNameObservable;
    //@FXML
    String boxName;

    BoxController(Box box) {
        boxNameObservable = box.boxName;

        boxNameObservable.observe(updatedName -> boxName = updatedName);
    }

    public void changeName(String str) {
        boxNameObservable.item = str;
        boxNameObservable.update();
    }
}

This is my first time posting here, so if I have done something wrong I'm happy to fix it! Thanks!

\$\endgroup\$
1

2 Answers 2

4
\$\begingroup\$

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();
}

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.
\$\endgroup\$
3
  • \$\begingroup\$ Great feedback, thank you! However my code does support multiple observed values doesn't it? Since the controller would simple create additional Observers to monitor additional values? \$\endgroup\$ Feb 1, 2022 at 12:05
  • \$\begingroup\$ My plan was actually to keep the update method public so the loophole you mention could be avoided by updating Observable from the outside. So you could get the item, make some changes, and then update the Observable wrapper. Is this resonable, or would you advise against it? \$\endgroup\$ Feb 1, 2022 at 12:38
  • \$\begingroup\$ Multiple observed values currently need multiple observers, you can't have one observer watch more than one value. Keeping the update() method for the loophole case is absolutely valid. And I'll add a paragraph recommending Javadoc to my answer. \$\endgroup\$ Feb 1, 2022 at 13:35
3
\$\begingroup\$

Unit tests

Dive into that; it is relative simple. Then you are doing TDD, Test Driven Development. You can keep those tests in the project, and future changes with regression errors then will be detected.

Java Usage

Compile against interfaces, implement with specialized classes.

List<Observer<Item>> observers = new ArrayList<>();

This way variables, method parameters and results have the most power, as you deal with a List, not a specific ArrayList. And you could always change the implementation of a variable. Also there are many utility functions that do this too. So: Collections.emptyList(), singletonList, asList; they can be assigned to a List, but would need to be copied into an ArrayList.

Observable.update should be private,

Also there is no need for wrapping an Observable in its own class. Especially if the Observable is public for adding listeners. Often there a component has several mutable properties, several Observable fields.

(On the other hand often values would merit their own class because of abstractions, operations, isValid() and so on.)

An Observable field should probably be final. Assigning an other Observable would lose all listeners = should be prevented.

public class Box {
    pubic final Observable<String> boxName = new Observable<>("hello");

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

    public String getBoxName() {
        return boxName.get();
    }
}

Listeners are weak references

The loose convention is to use for generic type parameter names one capital. That makes it clear it is not an imported class. Also one most often uses the name value:

The problem you have: every listener is kept alive, as long as the Observable exists. JavaFX for instance uses a WeakChangeListener. A WeakReference allows garbage collection, setting the referral to null.

public class Observable<V> {

    private V value;

    private final List<Observer<V>> observers = new ArrayList<>();

    public Observable(V value) {
        this.value = value;
    }

You could reduce the number of update events. Especially there is the case of reciprocal Observables which would lead to infinite control flow.

public void set(V value) {
    if (!Objects.equals(this.value, value) {
        this.value = value;
        update();
    }
}

The Objects.equals also deals with nulls.

The callback

It is still common to have interfaces used as lambda give the corresponding annotation. Should someone add a second method to the interface, an error results.

Existing change listeners in Java provide old and new value. Not necessary.

@FunctionalInterface
public interface Observer<V> {
    void update(V oldValue, V newValue);
}

Naming, onChange / ChangeListener, is irrelevant, till you want to fit in the java ecosystem, and should use already introduced names.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I just have a few questions for you. In your section about weak references the code has no weak reference, I can look up exactly how to use it but perhaps you should include it in the answer? Also, I don't quite understand what you are trying to say under your callback section. \$\endgroup\$ Feb 2, 2022 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.