0

(This question applies to the equivalent code in both Java and PHP)

I have a class like this:

class Foo {
    private int $bar;

    public function __construct(int $bar) {
        $this->bar = $bar;
    }

    public function add(int $bar) : Foo {
        return new Foo($this->bar + $bar);
    }
}

Due to certain reasons (the reason is irrelevant to this question), cloning the object is a faster implementation than calling new Foo, so I can change the add function to this:

public function add(int $bar) : Foo {
    $foo = clone $this; // equivalently, this.clone() in Java
    $foo->bar += $bar;
    return $foo;
}

If Foo was final, this should not result in any BC issues. Unfortunately, this is not the case. In fact, there is a subclass of Foo in the API:

class HeavyFoo extends Foo {
    private SomeHeavyObject $object;
}

SomeHeavyObject is a heavy class (as in it references lots of other data), and it should be garbage-collected as soon as unneeded to keep the memory usage low.

Meanwhile, there are lots of users writing this kind of code:

$added = getHeavyFoo()->add(3);
$someLongLivingStorage->insert($added);

With the assumption that Foo.add will return a light (only containing an int field) object, users insert the result of add to a storage that lives much longer than HeavyObject would. This is not exactly a memory leak, but greatly increases the memory consumption anyway.

However, according to OOP principles, returning a subclass instead of the superclass is totally fine.

Should I bump my semver major version because of this change?

(For those wondering why change to clone: I wrote a PHP extension such that if(ref_count_for($this) > 1) { $this = clone $this; }, so no need to allocate a new object if $this is a temporary that isn't referenced anywhere else, e.g. the intermediates in $foo->add(1)->add(2)->add(3))

3 Answers 3

1

You are changing what an existing API call does, so I'd say that yes, you need a major version bump.

Moreover, you say that there is a performance improvement but also a potential memory issue, so whether the change is a net improvement or not depends on the user use case.

If this performance gain is significant for your project I'd suggest that you add to the API instead.

You could add a new method called selfAdd (or something similar) or, if this behaviour is present in several different methods, a better solution might be to make it a different implementation of the original class'interface. This still lets the user choose the implementation better suited to their needs without cluttering the class interface.

2
  • 1
    As a side note, some objects may crash immediately upon cloning. An example is an object that implements __destruct (PHP) or finalize() (Java) that frees its corresponding field in some static (or otherwise shared) context. Cloning it would lead to double freeing.
    – SOFe
    Commented Feb 10, 2021 at 14:00
  • I like your idea about abstraction through an interface. Although in my case that'd be boilerplate, it's a good demonstration of how OOP could get powerful.
    – SOFe
    Commented Feb 10, 2021 at 14:01
3

I will assume that it is unspecified if HeavyFoo::add returns (a reference to) a HeavyFoo or a Foo object.

If that is the case, then technically you haven't changed the interface and don't need to bump the major version. The users that are relying on getting the lightweight object are relying on undocumented behavior.

On the other hand, if that undocumented behavior has been present for a significant time, a large proportion of your users depend on it and the change causes them a real inconvenience, then it would be a good idea to bump the major version anyway as a signal to your users that they must pay attention when upgrading to the new version.

1
  • 1
    A major version bump would also be a good opportunity to make the class final.
    – bdsl
    Commented Aug 16, 2020 at 21:31
2

Depending on the state of the object to clone, i would say yes. Since the constructor of a object may only fill some of the fields of the class. This will cause breakage in scenarios where fields get lazy loaded (caching for example). If thats not the case, as in the amount of fields in that class is equal to the amount of arguments in the constructor and thus all fields are final there is no breakage to be considered.

Another thing is to check for hooking implementations. PHP for example has __clone which some developers may have used for other purposes than you initially intended. Due to the nature of the planned change this hook would be executed and lead to undesired behaviour.

For the java end that shouldn't be a problem since you can hide the clone method from your implementation and thus hide it for overriding (which may cause further breakage though)

1
  • You are assuming the existence of other fields, which is sidetracking the question. In my case, it can be assumed that bar is the only (final) field in the class, so what you are saying is irrelevant.
    – SOFe
    Commented Aug 16, 2020 at 14:22

Your Answer

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

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