1
\$\begingroup\$

In the link https://stackoverflow.com/questions/4081858/about-java-cloneable is written that Cloneable is bad practise. How to rewrite the class below:

public class DataSource implements Cloneable {

  /**
   * Variable name
   */
  private String variableName;

  /**
   * Facility reference names
   */
  private List<String> facilities;

  /**
   * System reference names
   */
  private List<String> systems;

  /**
   * Device reference names that should be monitored
   */
  private List<String> devices;

  /**
   * Device Type
   */
  private String deviceType;

  /**
   * Device Type attribute
   */
  private String deviceTypeAttribute;

  /**
   * Time range
   */
  private TimeRange timeRange;

  /**
   * Retrieve data for all monitored devices
   */
  private Boolean allDevices;

  @Transient
  private List<SDIDevice> sdiDevices;

  public DataSource() {
    // for clone method
    facilities = new ArrayList<>();
    systems = new ArrayList<>();
    devices = new ArrayList<>();
    sdiDevices = new ArrayList<>();
  }

  public String getVariableName() {
    return variableName;
  }

  public void setVariableName(String variableName) {
    this.variableName = variableName;
  }

  public List<String> getFacilities() {
    return facilities;
  }

  public void setFacilities(List<String> facilities) {
    this.facilities = facilities;
  }

  public List<String> getSystems() {
    return systems;
  }

  public void setSystems(List<String> systems) {
    this.systems = systems;
  }

  public List<String> getDevices() {
    return devices;
  }

  public void setDevices(List<String> devices) {
    this.devices = devices;
  }

  public String getDeviceType() {
    return deviceType;
  }

  public void setDeviceType(String deviceType) {
    this.deviceType = deviceType;
  }

  public String getDeviceTypeAttribute() {
    return deviceTypeAttribute;
  }

  public void setDeviceTypeAttribute(String deviceTypeAttribute) {
    this.deviceTypeAttribute = deviceTypeAttribute;
  }

  public TimeRange getTimeRange() {
    return timeRange;
  }

  public void setTimeRange(TimeRange timeRange) {
    this.timeRange = timeRange;
  }

  public Boolean getAllDevices() {
    return allDevices;
  }

  public void setAllDevices(Boolean allDevices) {
    this.allDevices = allDevices;
  }

  public List<SDIDevice> getSdiDevices() {
    return sdiDevices;
  }

  public void setSdiDevices(List<SDIDevice> sdiDevices) {
    this.sdiDevices = sdiDevices;
  }

  public void addSdiDevice(SDIDevice sdiDevice) {
    if (sdiDevices == null) {
      sdiDevices = new ArrayList<>();
    }
    sdiDevices.add(sdiDevice);
    addDevice(sdiDevice.getReference());
  }

  private void addDevice(String reference) {
    if (devices == null) {
      devices = new ArrayList<>();
    }
    devices.add(reference);
  }

  @Override
  public DataSource clone() {
    DataSource clone = new DataSource();
    clone.allDevices = allDevices;
    clone.deviceType = deviceType;
    clone.deviceTypeAttribute = deviceTypeAttribute;
    clone.timeRange = timeRange.clone();
    clone.variableName = variableName;
    clone.devices = new ArrayList<>(devices);
    clone.sdiDevices = new ArrayList<>(sdiDevices);
    return clone;
  }
}
\$\endgroup\$
1
\$\begingroup\$

I would like to point out some things that I would watch out for, these may not be directly related to your question, but hopefully they are useful to you.

Alot of your comments are completely redundant and actually add clutter to your code.

some particularly bad examples are the comment Time range for the variable timeRange. Your comments should say why you're doing something not what you're doing. The code itself should say what you're doing.

Avoid returning references to the underlying mutable objects directly, in this case, your ArrayLists. Instead you should return a defensive copy.

public List<SDIDevice> getSdiDevices() {
    return sdiDevices;
}

will become

public List<SDIDevice> getSdiDevices() {
    return new ArrayList<>(sdiDevices);
}

This will not prevent the calling code from mutating the objects in the list themselves, but it will prevent the underlying list from being modified.

Now I'm not sure what your calling code looks like, but I would question the decision to add getters/setters for everything. I would recommend first making everything private, then only making a getter as you need it, and if you really need to make a setter, ask yourself again, then once more, and if you can't think of any alternative, you should then write a setter.

see https://stackoverflow.com/questions/9416245/how-to-avoid-getters-and-setters

There are plenty of arguments for and against getters & setters. You should read both to get a better understanding of when you should and shouldn't use them.

And when you do write a setter, don't assign the reference direction, assign a copy.

public void setFacilities(List<String> facilities) {
    this.facilities = facilities;
}

should become

public void setFacilities(List<String> facilities) {
    this.facilities = new ArrayList<>(facilities);
}

I'm not sure why you do a check for null in some of your add methods. The only time the list can be null is if the caller sets the list to null. If you want the setters, I would maybe throw a NullPointerException when trying to set one of the lists as null. (another danger of providing a setter)

I don't think there's a good reason for providing a getter/setter for any of your lists, instead you should provide an addDevice and removeDevice method. If you just do this, your List can never be null, provided to initialise it in the constructor (like you do already).

As for your actual question, I think Mibac provided a good alternative, just make a copy constructor instead.

public DataSource(DataSource otherSource){
    // copy all the attributes defensively
}

Hopefully this answer was useful for you.

\$\endgroup\$
1
\$\begingroup\$

As the stackoverflow question you mentioned already points out, the problems begin with subclass instances of DataSource. Your clone() method fails if it's called from a subclass instance, because it will always produce a DataSource, never the subclass.

So you can declare DataSource final and avoid the problem. But then you can as well use a copy-constructor public DataSource(DataSource original).

If you really want to stay with clone(), here are a few hints (maybe not complete...):

  • Start with DataSource clone = super.clone(); (assuming that DataSource directly inherits from the Object.clone() method, without any interfering clone() method from a superclass). This will take care of the subclass issue and will already make a shallow copy of your instance.

  • Then you only need to modify the fields where you want a fresh copy of the field contents, e.g. timeRange or devices.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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