7

I wish to alert the developer when he attempts to mutate an immutable object. The immutable object is actually an extension of a mutable object, and overrides the setters on said object to make it immutable.

Mutable base class: Vector3

public class Vector3 {
    public static final Vector3 Zero = new ImmutableVector3(0, 0, 0);

    private float x;
    private float y;
    private float z;

    public Vector3(float x, float y, float z) {
        this.x = x;
        this.y = y;
        this.z = z;
    }

    public void set(float x, float y, float z) {
        this.x = x;
        this.y = y;
        this.z = z;
    }
}

Immutable version: ImmutableVector3

public final class ImmutableVector3 extends Vector3 {
    public ImmutableVector3(float x, float y, float z) {
        super(x, y, z);
    }

    //Made immutable by overriding any set functionality.
    @Override
    public void set(float x, float y, float z) {
        throw new Exception("Immutable object."); //Should I even throw an exception?
    }
}

My use case is as follows:

public class MyObject {
    //position is set to a flyweight instance of a zero filled vector.
    //Since this is shared and static, I don't want it mutable.
    private Vector3 position = Vector3.Zero;
}

Let's say that a developer on my team decides he needs to manipulate the position of the object, but currently it's set to the static, immutable Vector3.Zero shared vector instance.

It would be best if he knew ahead of time that he needs to create a new instance of a mutable vector if the position vector is set to the shared instance of Vector3.Zero:

if (position == Vector3.Zero)
    position = new Vector3(x, y, z);

But, let's say he doesn't check this first. I think, in this case, it would be good to throw an Exception as shown above in the ImmutableVector3.set(x,y,z) method. I wanted to use a standard Java exception, but I wasn't sure which would be most fitting, and I'm not 100% convinced this is the best way to handle this.

The suggestions I've seen on (this question)[Is IllegalStateException appropriate for an immutable object? seem to suggest the following:

  • IllegalStateException - but it's an immutable object, thus only has a single state
  • UnsupportedOperationException - the set() method is unsupported since it overrides the base class, so this might be a good choice.
  • NoSuchElementException - I'm not sure how this would be a good choice unless the method is considered an element.

Furthermore, I'm not even sure I should throw an exception here. I could simply not change the object, and not alert the developer that they are trying to change an immutable object, but that seems troublesome as well. The problem with throwing an exception in ImmutableVector3.set(x,y,z) is that set has to throw an Exception on both ImmutableVector3 and Vector3. So now, even though Vector3.set(x,y,z) will never throw an exception, it has to be placed in a try/catch block.

Questions
  • Am I overlooking another option besides throwing exceptions?
  • If exceptions are the best way to go, which exception should I choose?
crush
  • 16,713
  • 9
  • 59
  • 100
  • 2
    I'd go with `UnsuppoertedOperationException`. The `unmodifiable*` decorators from `Collection` are a good precedent. – kiheru Aug 29 '13 at 17:41
  • @crush Are you solving a *known* performance issue by making Vector3D mutable? If not, just make it immutable. Let people construct a new instance when they want new values. Known performance issue = you did performance testing/analysis, and it's the bottleneck. – Eric Stein Aug 29 '13 at 17:48
  • @EricStein the values of the Vector3 will be changing every second or more frequently for hundreds of thousands of objects in my scope. I am under the, possibly false, presumption that creating a new Vector3 object each time I need to change the position of an object would eventually cause issues with GC pauses and/or be slower than simply setting the new position on an existing object. – crush Aug 29 '13 at 17:51
  • @crush I can't promise it won't be an issue, but Java's pretty good at dealing with short-lived objects (I blame Dimension). You may want to try it and see if it blows up your code first. – Eric Stein Aug 29 '13 at 17:54
  • @EricStein Actually, I just recalled this fact, that another limitation here with making Vector3 immutable is that JPA does not allow immutable objects to be serialized, and we are using JPA for our db/serialization abstraction layer. – crush Aug 29 '13 at 17:56
  • @crush That's the end of that party. This whole hubaloo is for one instance, right? The zero vector? Throw UnsupportedOperationException, as everyone else has (rightly) said. – Eric Stein Aug 29 '13 at 18:00
  • I have a whole handful of similar static, sharable, flyweight instances like that, but essentially yes. – crush Aug 29 '13 at 18:05

4 Answers4

11

Throwing an exception is usually the way to go, and you should use UnsupportedOperationException.

The Java API itself does recommend this in the collections framework:

The "destructive" methods contained in this interface, that is, the methods that modify the collection on which they operate, are specified to throw UnsupportedOperationException if this collection does not support the operation.

An alternative if you create the full class hierarchy is to create a super class Vector that is not modifiable and has no modifying methods like set(), and two sub-classes ImmutableVector (that guarantees immutability) and MutableVector (that is modifiable). This would be better because you get compile-time safety (you can't even try to modify an ImmutableVector), but depending on the situation might be over-engineered (and you might end up with many classes).

This approach was for example chosen for the Scala collections, they all exist in three variations.

In all cases where you want to modify the vector, you use the type MutableVector. In all cases where you don't care about modifying and just want to read from it, you just use Vector. In all cases where you want to guarantee immutability (e.g., a Vector instance is passed into a constructor and you want to ensure that nobody will modify it later) you use ImmutableVector (which would usually provide a copy method, so you just call ImmutableVector.copyOf(vector)). Down-casting from Vector should never be necessary, specify the correct sub-type that you need as type instead. The whole point of this solution (which needs more code) is about being type safe and with compile-time checks, so down-casting destroy that benefit. There are exceptions, though, for example the ImmutableVector.copyOf() method would, as an optimization, usually look like this:

public static ImmutableVector copyOf(Vector v) {
  if (v instanceof ImmutableVector) {
    return (ImmutableVector)v;
  } else {
    return new ImmutableVector(v);
  }
}

This is still safe and gives you one further benefit of immutable types, namely the avoidance of unnecessary copying.

The Guava library has a large collection of immutable collection implementations that follow these pattern (although they still extend the mutable standard Java collection interfaces and thus provide no compile-time safety). You should read the description of them to learn more about immutable types.

The third alternative would be to just have on immutable vector class, and no mutable classes at all. Very often mutable classes used for performance reasons are a premature optimization, as Java is very good in handling lots of small short-lived temporary objects (due to a generational garbage collector, object allocation is extremely cheap, and object cleanup can even be without any cost in the best case). Of course this is no solution for very large objects like lists, but for a 3-dimensional vector this is surely an option (and probably the best combination because its easy, safe, and needs less code than the others). I do not know any examples in the Java API of this alternative, because back in the days when most parts of the API were created, the VM was not that optimized and so too many objects were maybe a performance problem. But that should not prevent you from doing so today.

Philipp Wendler
  • 11,184
  • 7
  • 52
  • 87
  • Would the super class still have a `set()` method on it? Without the `set()` method on the base or an interface, I will have to cast up to `MutableVector` in order to access the setter, which also means checking if `position` is an instance of `MutableVector` first. – crush Aug 29 '13 at 17:48
  • In this case, though, I want to mutate the instance only if it's not set to an Immutable instance first. If it's an Immutable instance, then I want to create a new Mutable instance, and assign that to `position`. Thus, I'd always have to cast to `MutableVector` from then on in order to `set` the vector. – crush Aug 29 '13 at 18:04
  • Baring performance issues, you should strive to just have an immutable class. This will have the added benefit of avoiding a base/immutable/mutable hierarchy every time you get into this situation. – Daniel Kaplan Aug 29 '13 at 18:11
  • 1
    In this case, you can (and need to) cast, though I really do not like that kind of design. – Philipp Wendler Aug 29 '13 at 18:11
  • Problem with making them all immutable is my hands are tied with JPA not allowing serialization of final fields (which is mind boggling to me, but whatever). – crush Aug 29 '13 at 18:23
  • I am not sure whether this helps you, but there is a SO question about JPA and immutable types: http://stackoverflow.com/questions/7222366/immutable-value-objects-and-jpa – Philipp Wendler Aug 29 '13 at 18:25
  • I wanted to thank you for all the information and research on this. I've decided to go with a bit of what you have suggested along with a suggestion by tieTYT above. I will use an interface for `Vector3`, but place `set()` on it. It will return a `Vector3` instance. That instance will be `this` on `MutableVector3` and `new MutableVector3(x,y,z)` on `ImmutableVector3`. This will help to fascilitate the flyweight model which I wish to receive. I can't decide which answer to accept, though, since both of you contributed equally to my solution in my estimation. – crush Aug 29 '13 at 18:39
  • I gave you the accept because your answer is the most verbose, and covers many approaches. – crush Aug 29 '13 at 18:54
3

You should throw a compiler error if you try to mutate an immutable class.

Immutable classes should not extend mutable ones.

SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
  • 1
    Are you saying: Extract an interface from the mutable class and have the immutable class implement that interface instead? – Sotirios Delimanolis Aug 29 '13 at 17:41
  • I wasn't too sure about extending a mutable class, and trying to make it immutable. Do you have any further suggestions on how I could handle the above situations? – crush Aug 29 '13 at 17:41
  • I would rather Java supported `const` methods instead, which would go a long way towards solving this nonsense, but it's too late for that. :-) – C. K. Young Aug 29 '13 at 17:43
3

Given your situation, I think you should throw an UnsupportedOperationException. Here's why:

package com.sandbox;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Sandbox {

    public static void main(String[] args) throws IOException {
        List<Object> objects = Collections.unmodifiableList(new ArrayList<Object>());
        objects.add(new Object());
    }

}

This will throw a UnsupportedOperationException and is a very similar situation to yours.

But an even better thing to do (which may not be possible) is to design your immutable classes so there isn't a mutating method to call in the first place. As a developer, I don't want to learn at runtime that I can't use a method, I want to learn at or before compile time.

Your set method should return a new ImmutableVector which doesn't mutate anything.

You might want to take @SotiriosDelimanolis's idea, and make both classes implement an interface without a setter and then have your code pass around the interface. Again, this may not be possible given your legacy code.

Daniel Kaplan
  • 62,768
  • 50
  • 234
  • 356
  • I've been considering SotiriosDelimanolis's suggestion. Wouldn't I be forced to cast up to `MutableVector3` in order to mutate it? That would require an additional check as well: `if (position instanceof MutableVector3)` I believe. – crush Aug 29 '13 at 17:46
  • @crush You *might*. It depends on your legacy code. If you have code that depends on the implementation the mutable methods of the vector, then you'd have to. You might be able to refactor the mutable vector's `set` method so it returns its own type. Then have all legacy code use the result of the setter. Then your legacy code will depend on your new interface rather than the old implementation. – Daniel Kaplan Aug 29 '13 at 17:49
  • Something like `MutableVector3`, `public Vector3 set(x,y,z) { /*...*/ return this; }` and on `ImmutableVector3`,`public Vector3 set(x,y,z) { /*...*/ return new Vector3(x,y,z);`? – crush Aug 29 '13 at 17:54
  • @crush yes. If you make everything in legacy code that calls `MutableVector3.set` always do this: `x.myMutableVector = x.myMutableVector.set(x, y, z);` then you should be able to replace all implementations with your `ImmutableVector3` and completely remove your `MutableVector3`. Unfortunately, the compiler won't help you if you forget to say `x.myMutableVector = ` – Daniel Kaplan Aug 29 '13 at 17:58
  • I really like your suggestion of returning an instance from the `set` method. That simplifies my code even, and facilitates the flyweight model! When someone tries to set on the immutable special case, they will get a mutable Vector3 with the values they set that can then be used from then on. This, along with an Interface/Mutable/Immutable objects is there route I've decided to take! I'm having trouble deciding whose answer to accept, though, since you both aided equally to my solution. – crush Aug 29 '13 at 18:37
  • @crush That's not the route I was suggesting, but if that's what works best for you it might be viable. I think `set` on `ImmutableVector` should return another `ImmutableVector`. – Daniel Kaplan Aug 29 '13 at 18:49
  • In most cases, perhaps, but the way I'm using it, it's best that it returns a Mutable Vector. I'll be sure to document it appropriately. Thanks. – crush Aug 29 '13 at 18:54
1

Immutable classes should not derive from concrete mutable classes, nor vice versa. Instead, you should have a ReadableFoo abstract class with concrete MutableFoo and ImmutableFoo derivatives. That way a method which wants an object it can mutate can demand MutableFoo, a method which wants an object whose present state can be persisted merely by persisting a reference to the object can demand ImmutableFoo, and a method which won't want to change an object state and won't care what the state does after it returns can freely accept ReadableFoo without having to worry about whether they're mutable or immutable.

The trickiest aspect is deciding whether it's better to have the base class include a method to check whether it's mutable along with a "set" method which will throw if it isn't, or have it simply omit the "set" method. I would favor omitting the "set" method, unless the type includes features to facilitate a copy-on-write pattern.

Such a pattern may be facilitated by having the base class include abstract methods AsImmutable, AsMutable, along with concrete AsNewMutable. A method which is given a ReadableFoo named George and wants to capture its present state can store to a field GeorgeField either George.AsImmutable() or George.AsNewMutable() depending upon whether it thinks it will want to modify it. If it does need to modify it, it can set GeorgeField = GeorgeField.AsMutable(). If it needs to share it, it can return GeorgeField.AsImmutable(); if it expects to share it many times before changing it again, it can set GeorgeField = GeorgeField.AsImmutable(). If many objects need to share the states ...Foos, and most of them don't need to modify them at all but some need to modify them a lot, such an approach may work out more efficiently than using mutable or immutable objects alone.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • Consider my example above. I have `MyObject` with the member `position` on it. `position` is a `Vector3`. Let's say that `Vector3` is a base class/interface, and I have `MutableVector3` and `ImmutableVector3` that derive from it. `position` is initially set to an instance of `ImmutableVector3`. It remains an `ImmutableVector3` until `position` needs to change. At that point, a new `MutableVector3` will be created at the changed position, and used from then forward. However, if `set()` is omitted from the base/interface then anytime I wanted to `set()` I would need to check for immutability. – crush Aug 29 '13 at 18:11
  • @crush: If `set` methods were omitted from the base interface it would for sanity probably have to include one `AsSameMutable()` [either return *self* as mutable or throw exception]; if one wanted to call `SetXX` and `SetYY` on `GeorgeField`, one would need `GeorgeField=GeorgeField.AsMutable(); GeorgeField.AsSameMutable.SetXX(); GeorgeField.AsSameMutable.SetYY();`. If one used `AsMutable` rather than `AsSameMutable`, and omitted the first `AsMutable` erroneously thinking it had already happened, the attempts to set XX and YY would silently fail. – supercat Aug 29 '13 at 18:22