Prototype pattern re-factor

[panksy2k]

Hi,

The Prototype implementations where clone() is Overridden (ElfBeast, ElfMage etc) can rather not have implementation of clone(). Instead their respective parent class (Mage, Beast) can have a general implementation of clone which creates a new instance by doing a shallow copy (Object.clone()) and then subsequently type-cast to the type of class (Mage.class or Beast.class etc) it is cloning for.

This will have advantage of:

1. Removing verbosity among all implementations and dedicate the responsibility of cloning the object by their respective interfaces (abstract class).

2. Avoid using 'new' more than once and using only whilst cloning the prototype object that is passed in HeroFactoryImpl.

Regards,
panksy2k

[ghost]

Hi @panksy2k ,
Say we move the clone implementation from ElfBeast to Beast by doing this.

public Beast clone() throws CloneNotSupportedException {
return (Beast)( super.clone() );
}

Then if we call clone method on an object of ElfBeast, it will return an Object of type Beast, not ElfBeast. That is why, clone() implementation should be in ElfBeast class.

[jwheeler31]

@panksy2k @ghost,
Adding clone() method (copy() in this case), fights inheritance/polymorphism. Any sort of instantiation of an abstract class should throw a compile-time error. The current implementation should stand as is (only a concrete class is instantiated). If Beast were to have any attributes to be initialized, it should be done via constructor inheritance.

[iluwatar]

Under construction by https://github.com/jasciiz

[ohbus]

@jasciiz can you please share any progress you have made for this issue?

[ohbus]

Sure thing @barathgdkrish.

Please mention a timeline for when can we expect a PR against this issue.

[joshivrush]

Hey @ohbus, @barathgdkrish is still working on this or shall I start with this?

[ghost]

How about adding a default method in typed prototype abstract class itself?

@Slf4j
public abstract class Prototype<T> implements Cloneable {

  public T copy() {
    try {
      return (T) super.clone();
    } catch (CloneNotSupportedException e) {
      LOGGER.error("Failed to clone {}!", this.getClass().getName());
    }
    return null;
  }
}

Doing changes with respect to this class works fine and all test were passed. Not sure whether this is a good practice or not.

[ThexXTURBOXx]

Hi, if noone else is working on this, I could pick up this issue. Is that okay? :)

[iluwatar]

Yes, since there is no sign of PR from @barathgdkrish you can go ahead @ThexXTURBOXx

[yonatankarp]

There's a pr for the fix, but it seems that SonarCloud isn't happy with the try-catch of the clone in terms of coverage.
From checking online, it's considered anti-pattern to test this... Any suggestions?