2
\$\begingroup\$

I have an application that basically exists as a map and some controls. The controls change the look and feel of the map. I have a slider which is used to change the brightness, for example. At the moment there is only one map but more might be added in future. This seems to me like a good situation for observer pattern. However, the instance Controller of the slider exists far away from an instance of the map component so I'm not sure how exactly to implement it. To propagate a reference to the map through lots of function calls seems like poor design. (Perhaps it's due to poor design that they are referenced so far apart but I don't have control of that)

To solve this, I've written some glue code which sits in between. I've called them services for lack of a better name. They both work, but I'm not sure it's as elegant as it could be. I'm also not sure as to whether the Java Observer/Observable are adding anything at all in the more complicated service. I wonder whether there's another design pattern that may be better suited here, too.

Any help would be great.


Map:

public class Map
{
    public void setBrightness(int i)
    {
        System.out.println("Brightness set to: " + i);
    }
}

Controller:

public class Controller
{
    public void brightnessChanged()
    {
        // Create an "event" and fire it to all listeners
        BrightnessService.update(11111);
        OtherBrightnessService.update(55555);
    }
}

A simple brightness service:

import java.util.LinkedList;
import java.util.List;

public abstract class BrightnessService
{
    private static final List<Map> OBSERVERS;

    static
    {
        OBSERVERS = new LinkedList<>();
    }

    public static void register(Map map)
    {
        OBSERVERS.add(map);
    }
    public static void update(int i)
    {
        for (Map obs : OBSERVERS)
        {
            obs.setBrightness(i);
        }
    }
}

A possibly too complicated brightness service

import java.util.Observable;
import java.util.Observer;

public abstract class OtherBrightnessService
{
    static class MyObserver implements Observer
    {
        private Map map;

        public MyObserver(Map m)
        {
            map = m;
        }

        @Override
        public void update(Observable o, Object o1)
        {            
            map.setBrightness(((Brightness) o).get());
        }
    }
    static class Brightness extends Observable
    {
        private int brightness = 0;

        public void set(int i)
        {
            brightness = i;

            setChanged();
            notifyObservers();
        }

        public int get()
        {
            return brightness;
        }
    }

    static Brightness brightness;

    static
    {
        brightness = new Brightness();
    }

    public static void register(Map map)
    {
        MyObserver observer = new MyObserver(map);
        brightness.addObserver(observer);
    }
    public static void update(int i)
    {
        brightness.set(i);
    }
}

Main

public class ObserverTest
{
    public static void main(String[] args)
    {
        // Set up map
        Map m = new Map();
        BrightnessService.register(m);
        OtherBrightnessService.register(m);

        // Change the brightness, this would fire from a user event
        // and be implemented in a different file
        Controller c = new Controller();
        c.brightnessChanged();
    }
}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Why did you assume that only Map will ever want to observe the brightness value? As your code hints, you will possibly have not only multiple maps at one point, but also multiple points from which the brightness could be controlled. Which in return would also want to register with that service to update their displayed state in case of external control.


What you did forget to implement in that specific service, is an oldBrigthness != newBrightness test. If the control is set to the same value again, you don't want to propagate. Even less so if the scenario mentioned above occurs, and the self-adjustment of a brightness control could possibly broadcast another update.


I probably wouldn't use the old Observable and Observer classes, their interface predates the introduction of generics in 1.5, and as you noticed isn't even type safe yet.


You should revise on the API design on your service. You now have the odd situation where the BrightnessService needs to be aware of Map, while on the other side the controlling sites need to be aware of BrightnessService.

This is a rather unfortunate situation, since you now created a dependency chain from BrightnessSlider via BrightnessService to Map. This means a BrightnessSlider can't exist, without Map existing in the same application.

The solution to break this chain, is to make Map instead aware of BrightnessService, respectively define a generic interface in the package of BrightnessService which allows other components to register with this service. Now both the BrightnessSlider and the Map can exist and access the BrightnessService without the other component being part of the same application. As a side effect, this also solves the case where BrightnessSlider wants to observes the currently set brightness as well.


private static final List<Map> OBSERVERS;

Caps lock is for constants, not for mere static class variables. At least not if they can be considered "variable", and not constant.

Well, actually something as a static class variable shouldn't exist in your application to begin with. You should try to keep your classes always instantiable and free from side effects. This also applies to this service.

If you actually need to ensure that only a single instance of this service can exist, use the Singleton pattern. This is about the only exception to the rule where static final may be used to declare something else than a constant.

There are quite a few reasons for that, among others that the BrightnessSlider and Map classes usually would also want to hold a reference onto the BrightnessService they have been registered with (and be it only to provide a clean shutdown by unsuscribing themselves!).

It also allows you to move freely between Singleton and other techniques, such as dependency injection, should your code base require that later on. As the call sites already work on a specific instance of BrightnessService (with disregard as where they obtained it from!), there is little friction in changing that later on.

Furthermore, static methods should also be free of side effects, by convention. Performing constant calculations, or returning a constant result/reference? Good. Mutating some global state directly? Not good.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the advice. I'm glad that you agree that the Observables method isn't the way to go - it felt too clunky to me but I thought that was the "right" way to do it. Re: your last point, it's declared as final so shouldn't it be capitalised? Netbeans suggested that it should when I initially entered it in lowercase. \$\endgroup\$
    – Michael
    Sep 22, 2016 at 21:16
  • 1
    \$\begingroup\$ Netbeans assumes that it's a constant, interfered from the variable being declared as final static. However, you are not actually using it as a constant, but as a mutable container instead. This type of "global variable" is strongly discouraged in Java applications, and not covered by the naming standard. If you really need to make sure that only one instance of this service can exist, use the singleton pattern for that. But try to keep the service itself instantiable and also try to avoid static methods which manipulate data. \$\endgroup\$
    – Ext3h
    Sep 23, 2016 at 6:47
1
\$\begingroup\$

I suggest the use of an event system. The map subscribes to the event BrightnessChange and the slider will emit the event. This way, you can loosely couple any amount of sliders or switches to map-kind objects and any map will only understand those it's interested in. The broker is an EventBus-Object. There, you register handlers for Eventtypes and fire new events.

The danger with Events is that you can easily overdo the anount of events.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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