Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am seeking advice on the efficiency and long term implications of various ways of capturing modifications to instance variables that I have tried implementing. Essentially, this is setup:

In my framework, I originally had a class whose sub classes were only supposed to contain public instance variables - kind of like a struct in C. These classes were pretty much just for convenience in wrapping up multiple objects and primitives into one object.

Now, however, I need a way to individually listen for modifications to each of the instance variables. For example, when someone modifies the rotation field of a Transform object (featuring position, rotation, etc.), ideally a list of listeners would all be fired with some event that only pertains to the rotation variable - I don't care about the variables that weren't modified. Of course, you could add and remove listeners from this list.

My initial "solution" was an absolute hack. I essentially spin polled the variables I was interested in, stored the previous value, and examined if the new value matched the old one.

My first thought at remedying this was to change my setup from public instance variables to private variables with getters and setters for access (which I didn't do earlier for simplicity and code compactness). I would also maintain a list of listeners in every struct and notify each one when a setter was called.

However, this approach fell flat for two reasons:

First of all, what if a 3rd party extending this class forgot (or simply didn't know) to notify the listeners? How would I guarantee that they would implement the class in a way that wouldn't break other parts of the framework depending on these events? Another problem was that of mutable objects - even if clients correctly implemented getters and setters by notifying the listeners when a setter was called, there was no way of detecting when an object was retrieved with a getter and tinkered with internally.

My current approach seems from my view to be the most elegant, but runs into possible performance issues when autoboxing primitives. Here's the structure:

A DataComponent is the class that originally was intended to behave like a struct - it now contains a map of String keys to generic DataFields, which represent one instance variable.

DataComponent:

/**
 * A Component that holds a Map of keys to DataFields. Subclasses should not have any instance variables; rather, they
 * should add DataFields and allow the data to be accessed in this way.
 */

public abstract class DataComponent extends Component {
    private Map<String, DataField> m_dataFields;

    {
        m_dataFields = new HashMap<String, DataField>();
    }

    public DataComponent() {
        super();
    }

    /**
     * Puts a field for a specific key.
     * 
     * @param key
     * @param field
     */
    protected void putField(String key, DataField field) {
        m_dataFields.put(key, field);
    }

    /**
     * Removes a key from the Map.
     * 
     * @param key
     */
    public void removeField(String key) {
        m_dataFields.remove(key);
    }

    /**
     * Returns the DataField associated with the key.
     * 
     * @param key
     * @return
     */
    public DataField getField(String key) {
        return m_dataFields.get(key);
    }
}

DataField:

/**
 * A DataField for one T Object.
 * 
 * @param <T>
 */
public class DataField<T extends Object> extends MutationNotifier {
    private T m_data;

    private FieldMutationListener m_listener;

    {
        m_listener = new FieldMutationListener();
    }

    /**
     * Constructs an empty DataField.
     */
    public DataField() {

    }

    /**
     * Constructs a DataField with the specified initial data.
     * 
     * @param data
     */
    public DataField(T data) {
        m_data = data;
    }

    /**
     * Sets the Data in this DataField. If the data extends MutationNotifyer, a MutationListener is added to the Object.
     * 
     * @param data
     */
    protected void set(T data) {
        // If the old data extends MutationNotifier, meaning it's mutable, remove the listener that was previously
        // added.
        if (MutationNotifier.class.isAssignableFrom(m_data.getClass())) {
            ((MutationNotifier) m_data).removeMutationListener(m_listener);
        }

        // If the new data extends MutationNotifier, meaning it's mutable, add a listener so that we can notify our
        // listeners when the object is changed.
        if (MutationNotifier.class.isAssignableFrom(data.getClass())) {
            ((MutationNotifier) data).addMutationListener(m_listener);
        }

        m_data = data;

        // Since the old object was set to a new one, notify the listeners that the field has changed.
        this.notifyListeners();
    }

    /**
     * Gets the data in this field.
     * 
     * @return
     */
    protected T get() {
        return m_data;
    }

    private class FieldMutationListener implements MutationListener {
        @Override
        public void mutationOccurred() {
            DataField.this.notifyListeners();
        }
    }
}

MutationNotifier:

/**
 * A class that should be extended by a mutable object being added to a DataField. If the DataField sees that the Object
 * extends MutationNotifier, it will add a listener so it is notified of any changes.
 */
public class MutationNotifier {
    private List<MutationListener> m_listeners;

    {
        m_listeners = new ArrayList<MutationListener>();
    }

    /**
     * Constructs a MutationNotifier.
     */
    public MutationNotifier() {

    }

    /**
     * Adds a listener.
     * 
     * @param listener
     */
    public void addMutationListener(MutationListener listener) {
        m_listeners.add(listener);
    }

    /**
     * Removes a listener.
     * 
     * @param listener
     */
    public void removeMutationListener(MutationListener listener) {
        m_listeners.remove(listener);
    }

    /**
     * Notifies all listeners that a mutation occured.
     */
    public void notifyListeners() {
        for (MutationListener listener : m_listeners)
            listener.mutationOccurred();
    }
}

My concern with this is that former primitive instance variables now must be converted to full-blown objects. Given this framework is going to go into a game engine, this may be an issue (a possible way around this would be to offer a PrimitiveDataComponent that does the entire public instance variable thing without the embellishments - if you care about performance and don't need to listen for changes).

However, does anyone think autoboxing will reduce performance enough to make this necessary? Is there another, more efficient way of getting the effect I want? Any general programming tips would also be appreciated.

share|improve this question

3 Answers 3

In my framework, I originally had a class whose sub classes were only supposed to contain public instance variables - kind of like a struct in C.

^^^ That, that's the source of your problem ^^^

Java is an object oriented language. Objects encapsulate their data, and the better Object designs make it impossible for other classes to directly access the internal data.

Your system, you say, has a Transform class, with some fields including a rotation. It looks something like:

public class Transform {

    public double rotation = 0.0;

    ....

}

And, anyone can change the rotation.

The standard system used in Java is to make that rotation private, and to provide getters, and setters for it:

public class Transform {

    private double rotation = 0.0;

    public void setRotation(double newrotation) {
        this.roation = newrotation;
    }

    public double getRotation() {
        return rotation;
    }

    ....

}

Now, this is an encapsulated object.

To extend this object, so that interested parties can 'listen' for rotation events, you can have listeners registered with the class, with 'callbacks' that get fired when the rotation changes.... This is called the Observer Pattern. You should take a moment and read through all the common patterns, and you will discover a number of tools you can use for common problems.

public interface RotationListener {
    public void rotationChangedEvent(double oldrotation, double newrotation);
}

and, your class would have a listener mechanism added:

public class Transform {

    private double rotation = 0.0;
    private final List<RotationListener> rotationListeners = new ArrayList<>();

    public void setRotation(double newrotation) {
        double old = rotation;
        this.roation = newrotation;
        for (RotationListener ping : rotationListeners) {
            ping.rotationChanged(old, newrotation);
        }
    }

    public double getRotation() {
        return rotation;
    }

    public void addRotationListener(RotationListener toadd) {
        rotationListeners.add(toadd);
    }

    ....

}

Almost the entire Swing GUI framework has been built using this system. Consider all the Listeners available on JComponent and all the Events that can happen. Each Event has a corresponding listener available.

share|improve this answer
    
This works fine for primitive or immutable fields - but what about mutable object variables? Couldn't the client simply access the field using a getter and modify the object without ever calling set() or triggering the listener mechanism? –  Samuel Pfrommer Jul 7 at 1:37
    
There is no need for a listener on an immutable field ... ;-) As for mutable objects that your class contains, well, your option there is to hope it has, or add it if it does not have, a Listener infrastructure of its own. –  rolfl Jul 7 at 1:39
    
That's true, except for when you replace the object wholesale with another object :). –  Samuel Pfrommer Jul 7 at 2:01
    
Ahh, I missed that you meant that the data in the referenced instance was immutable, I though you mean that the reference and the data was immutable... e.g. I tyought you meant something like private final String myString; ... not private String myString. –  rolfl Jul 7 at 2:05
1  
+1, maybe it is worth mentionig that this is a concrete example of the Observer pattern (en.wikipedia.org/wiki/Observer_pattern). –  Dtex Jul 7 at 12:42

First of all, I think having public instance variables is a complete dead end. If you want to make sure that any time the instance variables are modified the listeners are notified, that needs to be encapsulated. It should be impossible or at least difficult to circumvent the listener notification process.

Approach 1: Use a mediator

From http://sourcemaking.com/design_patterns/mediator:

The mediator object: encapsulates all interconnections, acts as the hub of communication, is responsible for controlling and coordinating the interactions of its clients, and promotes loose coupling by keeping objects from referring to each other explicitly.

Approach 1: Use a facade

This is basically putting a guard at the gate of the castle, then submitting requests to that guard. Instead of calling setFoo on the object itself, you call setFoo on the Facade object. The Facade would then call setFoo and notify the listeners.

The downside of this approach is you wouldn't be able to make the setFoo methods public. All third-party code would have to go through the facade instead of going through the real objects.

I think @rolfl's answer is much better: call setFoo on the object and let the object notify listeners directly. I'm just including this approach for completeness but I don't recommend it.

Approach 2: final method with a hook for subclasses

If you're worried about someone overriding your method and not updating listeners, you can do this:

public final void setFoo(int foo) {
    this.foo = foo;
    // notify listeners here.
    subclassSetFoo(int foo);
}

protected void subclassSetFoo(int foo) { }

I forget the name for this design pattern. If you want to get fancy you can have a "before" hook and an "after" hook, but I would recommend not building both unless there's significant user demand.

Approach 3: Batch updates.

If you're getting performance issues, there's no rule that one change to the variable has to lead to one notification to listeners. You can batch updates and send a single "something changed" message, or batch updates to a single variable and only send out updates periodically. It's important that outside users don't have any easy way of circumventing this process. For example, you could have a dedicated thread which checks each variable periodically, and if that variable has changed it sends out an update to listeners of that variable. (I recommend a boolean "dirty" flag if you take this approach.)

Approach 4: Make instance variables package visible.

I don't recommend this unless there are no other good alternatives, because it's too easy to mess up and forget to notify the listeners. You'll still need to provide a public API, and you'll still need to design that API so that others can subclass it.

Final Thoughts

I wouldn't recommend converting primitives to full-blown objects and I don't see why that would be necessary. The most important things are to mediate access and ensure encapsulation, which can be done with one or more of the things I've listed above.

share|improve this answer
    
Could you explain how the mediator pattern would be applied in this case? I assume it would mediate between the DataComponent and the program accessing the data but I'm not sure how to implement it. Would every DataComponent supply a different mediator tailored to its fields? Would the mediator contain getters and setters, and then relay those function calls to the DataComponent? If so, what's the advantage of this system? –  Samuel Pfrommer Jul 7 at 1:54
    
I just edited my answer. I said "mediator" when I meant "facade". Yes, the facade(s) would have to have all the getters and setters and relay them to the DataComponent. What's the advantage? Hmm. Yeah it seemed like a good idea at the time. I no longer think using a facade is a good idea. –  Matthew Jacobs Jul 7 at 2:51

Your current code

Other answers provide you with plenty of alternative approaches, so I'll comment a bit about your current code.

private Map<String, DataField> m_dataFields;

Make that private final. Whenever possible, make variables final. The same goes for fields in your other classes.

Prefixing fields with m_ is something I would not recommend. Although I've seen a recommendation about that somewhere a year or so ago, about prefixing internal fields with m and static fields with s, it is not something I see a lot in today's frameworks. I would recommend to not prefix those names. Most IDEs today have a code highlighting feature to separate between these.


{
    m_dataFields = new HashMap<String, DataField>();
}

This part can/should be moved to the same line as your field declaration, making it:

private final Map<String, DataField> m_dataFields = new HashMap<String, DataField>();

Or, if you use Java 7 you can use:

private final Map<String, DataField> m_dataFields = new HashMap<>();

public DataComponent() {
    super();
}

Well that's one useless constructor. A public constructor with no arguments is always available if no constructors has been provided. And super() is automatically being invoked as long as the superclass only has one default constructor.


This code smells a bit...

    if (MutationNotifier.class.isAssignableFrom(m_data.getClass())) {
        ((MutationNotifier) m_data).removeMutationListener(m_listener);
    }

You could do ìf (m_data instanceof MutationNotifier), this would be exactly the same as your current code. However, Beware of the instanceof operator. It often is an indication of a design flaw in your code. Going with one of the other suggested approaches is IMO better here.

In your JavaDoc for MutationNotifier you write:

A class that should be extended by a mutable object being added to a DataField.

This is an excellent time to mention favor composition over inheritance


Map<String, DataField> m_dataFields

You should get a compiler warning here that DataField is a raw type. You should make it Map<String, DataField<?>>. And as your DataField is stored non-generically in a map, I wonder how much use you have for the generics part of it.


It seems like this class can be entirely removed:

private class FieldMutationListener implements MutationListener {
    @Override
    public void mutationOccurred() {
        DataField.this.notifyListeners();
    }
}

And your m_listener removed as well and all references to it replaced by this. Your DataField already implements MutationListener, I see no point in wrapping those calls inside this class.


Summary

Overall, I'd go with one of the other suggestions you've received here. But even when you do so, I hope you've learned something from my comments above.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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