Changing an immutable class in legacy code

Question

I want to replace the (mutable) class

class OldValue {
  private int value;
  
  public OldValue(int value) { this.value = value; }
  
  public int getValue() { return value; }
  
  public void setValue(int value) { this.value = value; }
}

with the immutable class

class NewValue {
  private final int value;
  
  public NewValue(int value) { this.value = value; }
  
  public int getValue() { return value; }
  
  /* no setter, obviously */
}

In most parts of the code something like newValue.set(777) can be replaced by newValue = new NewValue(777). However, there are some legacy parts of the code with something like

class ManagementServiceProviderFacadeSingletonAbstractFactoryObserver {

  public void setTheValue(OldValue oldValue) {
    oldValue.setValue(555);
  }

}

class Data {
  private OldValue oldValue;
  
  public OldValue get() { return oldValue; }
}


class SomewhereElse {

  public void frobnicate(Data data, ManagementServiceProviderFacadeSingletonAbstractFactoryObserver x) {
    x.setTheValue(data.get());
  }
}

These legacy parts are very hard to change and we would like to adapt it as little as possible. Is there a way to write some kind of "evil" setter method that could be used in the legacy code? Something like

class EvilSetter {

  public static void evilSet(NewValue newValue, int x) {
    // temporarily make newValue.value both public and non-final, set it to x
  }
}

Is there some other way without losing the immutability and elegance of the new design?

You can change the private member's value through [reflection](https://docs.oracle.com/javase/tutorial/reflect/index.html). — Robby Cornelissen, Nov 17 '20 at 09:20
Or try to feed the legacy code some mutable wrapper around your immutable type. — Hulk, Nov 17 '20 at 09:22
Or leave the class mutable and mark the setter as `@Deprecated` — Alex Shesterov, Nov 17 '20 at 09:23
Or just get it over and done with and also fix your legacy code. — Robby Cornelissen, Nov 17 '20 at 09:24
I disagree with "no setter, obviously". Immutable objects can (and often should) have setters, they just can't mutate the internal state, and should yield new instances. In your case: `public NewValue setValue(int value) { return new NewValue(value); }` — Jeroen Steenbeeke, Nov 17 '20 at 09:24
@JeroenSteenbeeke That's not a setter, according to the JavaBeans specification. That's some sort of ill-named factory method. — Robby Cornelissen, Nov 17 '20 at 09:25
@JeroenSteenbeeke that would be a very unidiomatic name for that method in my view, and it would never pass a code review here. — Hulk, Nov 17 '20 at 09:26
Come to think of it, methods of this type usually use the prefix `with` rather than `set` in most codebases I've encountered. And mutator is probably a better name than setter — Jeroen Steenbeeke, Nov 17 '20 at 09:29
@JeroenSteenbeeke It's not a mutator either. It doesn't mutate anything. It's a factory method, and should either be declared `static` and named appropriately, or not exist at all because it offers nothing beyond what the constructor already offers. — Robby Cornelissen, Nov 17 '20 at 09:33
I fail to see how `static` is applicable here. The method in question is comparable to `String.substring` in scope — Jeroen Steenbeeke, Nov 17 '20 at 09:37
@JeroenSteenbeeke It's not at all comparable. `String.substring()` bases itself on the value of `String` instance on which it is called to create a new `String` instance. Your suggested method requires absolutely nothing from the instance on which it is called. It doesn't require an instance to be executed at all. Hence: `static`. — Robby Cornelissen, Nov 17 '20 at 09:42
In this particular case, yes, but generally, no. Suppose you have an immutable class with 8 fields, and you want to change 1 of those. Would you rather have a single 1 parameter method that yields a new instance with this mutated field, or an 8-parameter constructor where you manually have to pass each field from the old instance except the one you wish to mutate? — Jeroen Steenbeeke, Nov 17 '20 at 09:44
@JeroenSteenbeeke What you're describing is a builder pattern that accepts a "product" instance to initialize the builder. It's an interesting approach. — Robby Cornelissen, Nov 17 '20 at 09:57
I suppose a builder is the closest analogy, but it's not an exact fit — Jeroen Steenbeeke, Nov 17 '20 at 09:58
Why would a random class with no clear contract with `Data` suddenly set a value in it? Instead of finding a hack for making it work, it looks like your time will be better spent in refactoring the "legacy" code. — Abhijit Sarkar, Nov 17 '20 at 09:59

Hulk · Accepted Answer · 2020-11-17T11:00:14.680

I'd probably keep both types, leave the legacy code using the old type alone and provide adapter methods for converting between the two.

public class LegacyValueAdapter {

    public static OldValue toOldValue(NewValue newValue) {
        return new OldValue(newValue.getValue);
    }

    public static NewValue toNewValue (OldValue oldValue) {
        return new NewValue(oldValue.getValue);
    }

}

Clearly document that these should only be used for interfacing with the legacy code, mark as deprecated as needed. This clearly isn't ideal, but that applies to the entire situation.

If the class is more complicated (e.g. also has methods with some non-trivial behavior), it might be worth implementing OldValue by wrapping an instance of NewValue, but in this simple case that won't provide much benefit.

I strongly advise against any reflection-hacks, as they would invalidate most the benefits of using the immutable types in the first place. Don't give me a type that looks immutable, but then sneakily change the value somehow - that breaks expectations of users and hides the risks for race-conditions.

Changing an immutable class in legacy code

1 Answers1