Return type of jmeClone()

Development
1

I’ve noticed many places in the Engine where jmeClone() methods specify Object as their return type. I understand why this was done in “JmeCloneable.java”. However, I don’t see why it should be the case in any other class.

For instance, wouldn’t it be clearer to have (in “AnimComposer.java”)

    @Override
    public AnimComposer jmeClone() {
        try {
            AnimComposer clone = (AnimComposer) super.clone();
            return clone;
        } catch (CloneNotSupportedException ex) {
            throw new AssertionError();
        }
    }

instead of the current

    @Override
    public Object jmeClone() {
        try {
            AnimComposer clone = (AnimComposer) super.clone();
            return clone;
        } catch (CloneNotSupportedException ex) {
            throw new AssertionError();
        }
    }

?

3 Likes
2

Agree with you.

1 Like
3

Not sure if I am mistaking this, but you can’t change the return type for @Overriden methods, so you’d loose the interface support, which I guess is the whole reason the return type is flawed, both here and in the jdk Clonable

2 Likes
4

I guess the Java Cloneable marker interface is the copy source?

5

I guess it should be possible if it is a subtype. For example see MorphControl:

1 Like
6

you can’t change the return type for @Overriden methods

“An overriding method can also return a subtype of the type returned by the overridden method.”

In Java, all classes are subtypes of Object.

2 Likes
7

@sgold sorry if I am wrong but could this break the compiled codes? (because the method signature has changed)

1 Like
8

I think it might break user classes that override jmeClone() implementations in JME. But only if they try to return Object themselves?

I think it’s too late to change this in v3.4. But it could plausibly be changed in v3.5 or 4.0.

9

No one should be calling jmeClone() directly, though. So what’s the point?

1 Like
10

Good question.

super.jmeClone() is sometimes invoked in the jmeClone() methods of new subclasses. jme3-core does this in 5 places.

Edit: For me, the issue is readability, not correctness.

11

But then they should be casting it to themselves and it doesn’t matter what they return. The pattern is the same for Java’s regular clone() method.

The issue I have with changing it is that it makes it easier to call it directly… which is already wrong. And the only “correct” reason to do it also isn’t correct since subclasses should be casting the result to themselves (or not at all if they don’t care).

1 Like
12

It’s always better to favor abstraction over concretion…unless applying restrictions (more strict than some annotations requiring fields from the same package), but as you have said that returning a subclass of the proposed interface function would not break the overriding, but we may refractor 2/3 of jme classes :grin:.

1 Like
13

I think this depends, if the user has implemented JmeClonable interface & used super.jmeClone() inside to get the default code then add his own code without overriding the method jmeclone directly, there would be no breaks, otherwise if he overrides directly w/o jmeClone he may break the code if he returns Object because the super class now doesn’t use Object, it uses Object+AnimComposer code.

1 Like
14

Reasons to do it: none

Reasons not to do it: it provides no benefit, makes it seem like this is a method that users should be calling, will confuse anyone trying to reason about what this method is for, might break existing code.

Show me one good example of where this would be helpful to anyone… so I can correct you on usage.

1 Like
15

I believe it would help someone reading the existing code, as in the “AnimComposer.java” example I posted at the start of this discussion. It would make it clearer what type of object the function returns.

16

Clearer to whom? The coder that shouldn’t be calling it or the coder that shouldn’t be calling it… or the coder that shouldn’t be calling it. No user code should ever call this method. If it were possible in Java then it would be private.

The super.jmeClone() argument isn’t a valid one because the caller should already be casting it to the local class and it makes no difference what it returns.

For everyone else, returning a real type only makes it look more like a method that code should actually be calling.

The fact that we are even having this conversation is only proving to me how much more confusing this method would be if it returned a real type… because somehow folks think it matters. Which seems like a clear misunderstanding of the method to me.

Anyone writing their own jmeClone() method will need to spend a few minutes understanding what it does and why or they will do it wrong.

1 Like
17

Since maybe I’m wrong about this. Let’s conduct a poll.

Everyone who started this thread thinking that jmeClone() needed a real return type… put yourself in that mindset and answer the following poll.

Why I thought/think jmeClone() needs a concrete return type:

  • Because when I’m calling it from my code to make a clone, I won’t have to cast.
  • Because when building my own jmeClone() method, I will know what to return.
  • Because when making a subclass, I won’t have to cast the super.jmeClone() call.
  • Other. (please explain)
  • I never thought it needed a concrete return type (just covering this case, too)

0 voters

1 Like
18

I think may be you can document the jmeClone() interface method with this option & let the user choose what he/she wants to do, so if he wants to keep it abstract so be it, otherwise he stills have the option to return a concrete subtype of the extended instance …may be…it won’t matter very much, but I am just giving out an option…

1 Like
19

Users can do whatever they want, of course.

But if we change method signatures in JME then it could break user code that depends on libraries that have not been compiled for that specific JME version.

2 Likes
20

Yeh ofc, I mean documentation for readability, to avoid confusion of why returning abstract not concrete type, as a guideline if he doesn’t already know.

1 Like