Cloning a Class In Java

Question

I have this class that implements Cloneable. I only need a shallow copy here. Can anyone point to what is wrong with the java compliance here.

public class EventSystem implements Cloneable{

    private String enrollmentId;
    private String requestId;
    private String tokenId;
    private Date eventAt;
    private Date loggedAt;
    private String appCardId;   
    private String fieldKey;
    private String fieldValue;
    private String trsDimCardIssuerId;
    private String trsDimCardProductId;
    private String trsDimAppEventLocationId;
    private String trsDimPaymentNetworkId;
    private String trsDimAppCardTypeId;
    private String trsTempLogId;



    public Date getEventAt() {
        return eventAt;
    }
    public void setEventAt(Date eventAt) {
        this.eventAt = eventAt;
    }
    public Date getLoggedAt() {
        return loggedAt;
    }
    public void setLoggedAt(Date loggedAt) {
        this.loggedAt = loggedAt;
    }
    public String getRequestId() {
        return requestId;
    }
    public void setRequestId(String requestId) {
        this.requestId = requestId;
    }
    public String getEnrollmentId() {
        return enrollmentId;
    }
    public void setEnrollmentId(String enrollemntId) {
        this.enrollmentId = enrollemntId;
    }
    public String getTokenId() {
        return tokenId;
    }
    public void setTokenId(String tokenId) {
        this.tokenId = tokenId;
    }
    public String getTrsDimCardIssuerId() {
        return trsDimCardIssuerId;
    }
    public void setTrsDimCardIssuerId(String trsDimCardIssuerId) {
        this.trsDimCardIssuerId = trsDimCardIssuerId;
    }
    public String getTrsDimCardProductId() {
        return trsDimCardProductId;
    }
    public void setTrsDimCardProductId(String trsDimCardProductId) {
        this.trsDimCardProductId = trsDimCardProductId;
    }
    public String getTrsDimAppEventLocationId() {
        return trsDimAppEventLocationId;
    }
    public void setTrsDimAppEventLocationId(String trsDimAppEventLocationId) {
        this.trsDimAppEventLocationId = trsDimAppEventLocationId;
    }
    public String getTrsDimPaymentNetworkId() {
        return trsDimPaymentNetworkId;
    }
    public void setTrsDimPaymentNetworkId(String trsDimPaymentNewtorkId) {
        this.trsDimPaymentNetworkId = trsDimPaymentNewtorkId;
    }
    public String getTrsDimAppCardTypeId() {
        return trsDimAppCardTypeId;
    }
    public void setTrsDimAppCardTypeId(String trsDimAppCardTypeId) {
        this.trsDimAppCardTypeId = trsDimAppCardTypeId;
    }
    public static long getSerialversionuid() {
        return serialVersionUID;
    }
     @Override
    public Object clone() throws CloneNotSupportedException {
            return super.clone();
        }
    public String getTrsTempLogId() {
        return trsTempLogId;
    }
    public void setTrsTempLogId(String trsTempLogId) {
        this.trsTempLogId = trsTempLogId;
    }
    public String getAppCardId() {
        return appCardId;
    }
    public void setAppCardId(String appCardId) {
        this.appCardId = appCardId;
    }
    public String getFieldKey() {
        return fieldKey;
    }
    public void setFieldKey(String fieldKey) {
        this.fieldKey = fieldKey;
    }
    public String getFieldValue() {
        return fieldValue;
    }
    public void setFieldValue(String fieldValue) {
        this.fieldValue = fieldValue;
    }
}

Is there a problem with String copy here.

I'm pretty sure the documentation strongly advises against the use of clone() in any circumstances. You can implement a copy constructor if you really need another instance. — M B, Nov 11 '16 at 19:54
@pathfinderelite - It is working fine. My only problem is that it stands again java compliance. But this serves my purpose of shallow copying. — nnc, Nov 11 '16 at 19:56
I would say ... this rather looks like a design problem. A) a class with so many fields ... is probably doing way too many things, too B) sure you need all those setters there? And please note: most of your properties are strings. You could as well use a Map here ... but dont get me wrong: maybe your code is fine as it is; but well, if it would be mine; I would dramatically rework it. — GhostCat, Nov 11 '16 at 19:59
This is a DAO class and it is designed to have these. Anyhow the question is, do we still have problems with cloning this ? — nnc, Nov 11 '16 at 20:07

score 1 · Answer 1 · answered Nov 11 '16 at 20:29

Your String fields aren't a problem. Your Date fields are.

When you clone an EventSystem instance, each of its fields points to exactly the same object as source object’s corresponding field. Thus, a cloned instance’s enrollmentId field points to the same String object as the original instance’s enrollmentId.

But that’s perfectly okay. You can safely share String objects, because they’re immutable. A String object cannot be altered. You can change value of a field which holds a String, but the String object itself can never change.

However, Date objects can be changed. This means the clone is not truly independent of the source instance. They both refer to the same mutable object, so if that object is changed for just one of the two EventSystem instances, both instances will see the changes, which can lead to some insidious bugs. Consider this code:

Calendar calendar = Calendar.getInstance();
calendar.set(1969, Calendar.JULY, 20, 22, 56, 0);
Date moonLanding = calendar.getTime();

EventSystem e1 = new EventSystem();
e1.setEventAt(moonLanding);

// Prints Sun Jul 20 22:56:00 EDT 1969
System.out.println(e1.getEventAt());

EventSystem e2 = (EventSystem) e1.clone();

// Both e1 and e2 have references to the same Date object, so changes
// to that Date object are seen in both objects!
e2.getEventAt().setTime(System.currentTimeMillis());

// You might expect these to be different, since we only changed
// e2.getEventAt(), but they're the same.
System.out.println(e1.getEventAt());
System.out.println(e2.getEventAt());

One way to resolve this is to use a common object-oriented technique known as defensive copying:

public Date getEventAt() {
    return (eventAt != null ? (Date) eventAt.clone() : null);
}

public void setEventAt(Date eventAt) {
    this.eventAt = (eventAt != null ? (Date) eventAt.clone() : null);
}

public Date getLoggedAt() {
    return (loggedAt != null ? (Date) loggedAt.clone() : null)
}

public void setLoggedAt(Date loggedAt) {
    this.loggedAt = (loggedAt != null ? (Date) loggedAt.clone() : null);
}

This prevents any other classes from directly modifying the internal Date field.

Another, less safe option is to clone the Date fields in your clone method:

@Override
public Object clone() throws CloneNotSupportedException {
    EventSystem newInstance = (EventSystem) super.clone();

    if (newInstance.eventAt != null) {
        newInstance.eventAt = (Date) newInstance.eventAt.clone();
    }
    if (newInstance.loggedAt != null) {
        newInstance.loggedAt = (Date) newInstance.loggedAt.clone();
    }

    return newInstance;
}

Cloning a Class In Java

1 Answers1