How to write a method using single responsibility princpile?

Question

I have a below class in which isValid method is being called.

• I am trying to extract few things from Record object in the isValid method. And then I am validating few of those fields. If they are valid, then I am populating the holder map with some additional fields and then I am populating my DataHolder builder class and finally return the DataHolder class back.
• If they are not valid, I am returning null.

Below is my class:

public class ProcessValidate extends Validate {
  private static final Logger logger = Logger.getInstance(ProcessValidate.class);

  @Override
  public DataHolder isValid(String processName, Record record) {
    Map<String, String> holder = (Map<String, String>) DataUtils.extract(record, "holder");
    String deviceId = (String) DataUtils.extract(record, "deviceId");
    Integer payId = (Integer) DataUtils.extract(record, "payId");
    Long oldTimestamp = (Long) DataUtils.extract(record, "oldTimestamp");
    Long newTimestamp = (Long) DataUtils.extract(record, "newTimestamp");
    String clientId = (String) DataUtils.extract(record, "clientId");

    if (isValidClientIdDeviceId(processName, deviceId, clientId) && isValidPayId(processName, payId)
        && isValidHolder(processName, holder)) {
      holder.put("isClientId", (clientId == null) ? "false" : "true");
      holder.put("isDeviceId", (clientId == null) ? "true" : "false");
      holder.put("abc", (clientId == null) ? deviceId : clientId);
      holder.put("timestamp", String.valueOf(oldTimestamp));

      DataHolder dataHolder =
          new DataHolder.Builder(record).setClientId(clientId).setDeviceId(deviceId)
              .setPayId(String.valueOf(payId)).setHolder(holder).setOldTimestamp(oldTimestamp)
              .setNewTimestamp(newTimestamp).build();
      return dataHolder;
    } else {
      return null;
    }
  }

  private boolean isValidHolder(String processName, Map<String, String> holder) {
    if (MapUtils.isEmpty(holder)) {
      // send metrics using processName
      logger.logError("invalid holder is coming.");
      return false;
    }
    return true;
  }

  private boolean isValidpayId(String processName, Integer payId) {
    if (payId == null) {
      // send metrics using processName     
      logger.logError("invalid payId is coming.");
      return false;
    }
    return true;
  }

  private boolean isValidClientIdDeviceId(String processName, String deviceId, String clientId) {
    if (Strings.isNullOrEmpty(clientId) && Strings.isNullOrEmpty(deviceId)) {
      // send metrics using processName     
      logger.logError("invalid clientId and deviceId is coming.");
      return false;
    }
    return true;
  }
}

Is my isValid method doing lot of things? Can it be broken down in multiple parts? Or is there any better way to write that code?

Also I don't feel great with the code I have in my else block where I return null if record is not valid. I am pretty sure it can written in much better way.

Update:

In my case this is what I was doing. I am calling it like this:

Optional<DataHolder> validatedDataHolder = processValidate.isValid(processName, record);
if (!validatedDataHolder.isPresent()) {
    // log error message
}
// otherwise use DataHolder here

So now it means I have to do like this:

boolean validatedDataHolder = processValidate.isValid(processName, record);
if (!validatedDataHolder) {
    // log error message
}
// now get DataHolder like this?    
Optional<DataHolder> validatedDataHolder = processValidate.getDataHolder(processName, record);

Answer 1

You are correct isValid() is doing too many things. But not only that, when most of us see a method that is called isValid() - we expect a boolean value to be returned. In this case, we're getting back and instance of DataHolder which is counterintuitive.

Try to split the things that you do in the method, for example:

public static boolean isValid(String processName, Record record) {
    return isValidClientIdDeviceId(processName, record) &&
           isValidPayId(processName, record) &&
           isValidHolder(processName, record);
}

and then construct DataHolder in a different method, say:

public static Optional<DataHolder> getDataHolder(String processName, Record record) {
    Optional<DataHolder> dataHolder = Optional.empty();
    if (isValid(processName, record)) {
        dataHolder = Optional.of(buildDataHolder(processName, record));
        // ...
    }
    return dataHolder;
}

It will make your program easier to both read and maintain!