How to clone old builder to make a new builder object?

I have a builder class which I am using in one of my project.

Let's say I have metricA as builder based on below class.
I need to make a new builder metricB based on metricA by cloning metricA so that metricB contains all the values which were already there in metricA.

In the constructor of MetricHolder I am initializing some fields (which are not set directly) basis on fields that have been set already.

clientTypeOrPayId - I am initializing this field. If payId is present, then I will set this value or I will set clientType.
clientKey - I am initializing this field as well in the same constructor.
And most importantly, I am putting few mandatory fields in the clientPayload map. I am not sure what is the right way to do that. But I need to add is_clientid and is_deviceid into the map. (In general I am adding few more fields).
And then in the last of the constructor, I am calculating latency difference and sending it to some other system.

Below is my class:

public final class MetricHolder {
  private final String clientId;
  private final String deviceId;
  private final String payId;
  private final String clientType;
  private final String clientTypeOrPayId;
  private final Schema schema;
  private final String schemaId;
  private final String clientKey;
  private final Map<String, String> clientPayload;
  private final Record record;
  private final long clientCreateTimestamp;
  private final long clientSentTimestamp;

  private MetricHolder(Builder builder) {
    this.payId = builder.payId;
    this.siteId = builder.siteId;
    this.clientType = builder.clientType;
    this.clientId = builder.clientId;
    this.deviceId = builder.deviceId;
    this.schema = builder.schema;
    this.schemaId = builder.schemaId;
    // populating all the required fields in the map and make it immutable
    // not sure whether this is right?
    builder.clientPayload.put("is_clientid", (clientId == null) ? "false" : "true");
    builder.clientPayload.put("is_deviceid", (clientId == null) ? "true" : "false");
    this.clientPayload = Collections.unmodifiableMap(builder.clientPayload);
    this.clientTypeOrPayId = Strings.isNullOrEmpty(payId) ? clientType : payId;
    this.record = builder.record;
    this.clientKey = "process:" + System.currentTimeMillis() + ":"
                        + ((clientId == null) ? deviceId : clientId);
    this.clientCreateTimestamp = builder.clientCreateTimestamp;
    this.clientSentTimestamp = builder.clientSentTimestamp;
    // this will be called twice while cloning
    // what is the right way to do this then?
    SendData.getInstance().insert(clientTypeOrPayId,
        System.currentTimeMillis() - clientCreateTimestamp);
    SendData.getInstance().insert(clientTypeOrPayId,
        System.currentTimeMillis() - clientSentTimestamp);
  }

  public static class Builder {
    private final Record record;
    private Schema schema;
    private String schemaId;
    private String clientId;
    private String deviceId;
    private String payId;
    private String clientType;
    private Map<String, String> clientPayload;
    private long clientCreateTimestamp;
    private long clientSentTimestamp;

    // this is for cloning
    public Builder(MetricHolder packet) {
      this.record = packet.record;
      this.schema = packet.schema;
      this.schemaId = packet.schemaId;
      this.clientId = packet.clientId;
      this.deviceId = packet.deviceId;
      this.payId = packet.payId;
      this.clientType = packet.clientType;
      // make a new map and check whether mandatory fields are present already or not
      // and if they are present don't add it again.
      this.clientPayload = new HashMap<>();
      for (Map.Entry<String, String> entry : packet.clientPayload.entrySet()) {
        if (!("is_clientid".equals(entry.getKey()) || "is_deviceid".equals(entry.getKey())) {
          this.clientPayload.put(entry.getKey(), entry.getValue());
        }
      }
      this.clientCreateTimestamp = packet.clientCreateTimestamp;
      this.clientSentTimestamp = packet.clientSentTimestamp;
    }

    public Builder(Record record) {
      this.record = record;
    }

    public Builder setSchema(Schema schema) {
      this.schema = schema;
      return this;
    }

    public Builder setSchemaId(String schemaId) {
      this.schemaId = schemaId;
      return this;
    }

    public Builder setClientId(String clientId) {
      this.clientId = clientId;
      return this;
    }

    public Builder setDeviceId(String deviceId) {
      this.deviceId = deviceId;
      return this;
    }

    public Builder setPayId(String payId) {
      this.payId = payId;
      return this;
    }

    public Builder setClientType(String clientType) {
      this.clientType = clientType;
      return this;
    }

    public Builder setClientPayload(Map<String, String> payload) {
      this.clientPayload = payload;
      return this;
    }

    public Builder setClientCreateTimestamp(long clientCreateTimestamp) {
      this.clientCreateTimestamp = clientCreateTimestamp;
      return this;
    }

    public Builder setClientSentTimestamp(long clientSentTimestamp) {
      this.clientSentTimestamp = clientSentTimestamp;
      return this;
    }

    public MetricHolder build() {
      return new MetricHolder(this);
    }
  }

    // getters
}

Question:-

Below is how I make metricA builder object:

MetricHolder metricA = new MetricHolder.Builder(record).setClientId("123456").setDeviceId("abcdefhg")
                .           setPayId("98765").setClientPayload(payloadMapHolder).setClientCreateTimestamp(createTimestamp)
                            .setClientSentTimestamp(sentTimestamp).build();

Now this is how I clone the metricA object later on in the code when I get all other fields as shown below:

MetricHolder metricB = new MetricHolder.Builder(metricA).setSchema(schema).setSchemaId("345").build();

I see two problem now:

First of all, my SendData.getInstance() line in the MetricHolder constructor will be called twice. First is when I make metricA and second when I make metricB by cloning metricA. But I just want to call it only once when I try to create metricA builder object? How can I make this possible?
Second is, the way I am populating clientPayload map with two mandatory fields in the MetricHolder constructor doesn't look right to me. Is there any other better way to do the same thing?

I guess the whole problem is happening because the way I am cloning metricA to make a metricB builder object? What is the best way to do this? I want to achieve above two things but in a right way.

But I just want to call it only once when I try to create metricA builder object? How can I make this possible?

The most straightforward way is to have a flag in the builder indicating whether it was created by Record or by cloning:

class Builder {
  final boolean cloned;

  Builder(MetricHolder packet) {
    this.cloned = true;
    // ...
  }

  Builder(Record record) {
    this.cloned = false;
    // ...
  }
}

Then, in the constructor of MetricHolder:

if (!builder.cloned) {
  SendData.getInstance().whatever();
}

But it's worth pointing out that making this call to SendData is an example of doing too much work in the constructor. You should think carefully about whether you really want to be making this call in the constructor, or whether you can factor that out into another method.

Second is, the way I am populating clientPayload map with two mandatory fields in the MetricHolder constructor doesn't look right to me. Is there any other better way to do the same thing?

You've misunderstood the "unmodifiable" bit of using Collections.unmodifiableMap: it's only an unmodifiable view of the map parameter; you can still modify the underlying map.

Here's a JUnit test to demonstrate:

Map<String, String> original = new HashMap<>();
original.put("hello", "world");

// Obviously false, we just put something into it.
assertFalse(original.isEmpty());

Map<String, String> unmodifiable = Collections.unmodifiableMap(original);
// We didn't modify the original, so we don't expect this to have changed.
assertFalse(original.isEmpty());
// We expect this to be the same as for the original.
assertFalse(unmodifiable.isEmpty());

try {
  unmodifiable.clear();
  fail("Expected this to fail, as it's unmodifiable");
} catch (UnsupportedOperationException expected) {}

// Yep, still the same contents.
assertFalse(original.isEmpty());
assertFalse(unmodifiable.isEmpty());

// But here's where it gets sticky - no exception is thrown.
original.clear();
// Yep, we expect this...
assertTrue(original.isEmpty());

// But - uh-oh - the unmodifiable map has changed!
assertTrue(unmodifiable.isEmpty());

The thing is that the map is only unmodifiable if there is no other reference to it hanging around: if you don't have a reference to original, unmodifiable actually is unmodifiable; otherwise, you can't rely upon the map never changing.

In your particular case, you are simply wrapping the clientPayload map in your unmodifiable collection. So, you're overwrite values for previously-constructed instances.

For example:

MetricHolder.Builder builder = new MetricHolder.Builder();
MetricHolder first = builder.build();
assertEquals("false", first.clientPayload.get("is_clientid"));
assertEquals("true", first.clientPayload.get("is_deviceid"));

builder.setClientId("").build();
// Hmm, first has changed.
assertEquals("true", first.clientPayload.get("is_clientid"));
assertEquals("false", first.clientPayload.get("is_deviceid"));

The correct approach is not to wrap builder.clientPayload. Take a copy of the map, modify it, and then wrap with unmodifiableMap:

{
  Map<String, String> copyOfClientPayload = new HashMap<>(builder.clientPayload);
  copyOfClientPayload.put("is_clientid", (clientId == null) ? "false" : "true");
  copyOfClientPayload.put("is_deviceid", (clientId == null) ? "true" : "false");
  this.clientPayload = Collections.unmodifiableMap(copyOfClientPayload);
}

The surrounding {} aren't strictly necessary, but they restrict the scope of copyOfClientPayload, so you can't accidentally reuse it later in the constructor.

Recommended topics

Hot tags