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!

Answer 2

I think things start with naming here.

As alfasin is correctly pointing out, the informal convention is that a method named isValid() should return a boolean value. If you really consider returning a DataHolder; my suggestion would be to change name (and semantics a bit), like this:

DataHolder fetchHolderWithChecks(String processName, Record ...)

And I wouldn't return null - either an Optional; or simply throw an exception. You see, don't you want to tell your user about that error that occured? So when throwing an exception, you would have a mean to provide error messages to higher levels.

On validation itself: I often use something like this:

interface OneAspectValidator {
  void check(... // if you want to throw an exception
  boolean isValid(... // if you want valid/invalid response

And then various implementations of that interface.

And then, the "validation entry point" would somehow create a list, like

private final static List<OneAspectValidator> validators = ...

to finally iterate that list to validate those aspects one by one.

The nice thing about that approach: you have the code for one kind of validation within one dedicated class; and you can easily enhance your validation; just by creating a new impl class; and adding a corresponding object to that existing list.

Answer 3

I know this might not be directly actionable, but the first thing you should do if you want to clean up this code is to use OO (Object-Orientation). If you are not using OO properly, then there is no point arguing the finer details of OO, like SRP.

What I mean is, I couldn't tell what you code is about. Your classnames are "ProcessValidate" (is that even a thing?), "Record", "DataHolder". That is pretty suspect right there.

The string literals reveal more about the domain ("payId", "deviceId", "clientId") than your identifiers, which is not a good sign.

Your code is all about getting data out of other "objects" instead of asking them to do stuff (the hallmark of OO).

Summary: Try to refactor the code into objects that reflect your domain. Make these objects perform tasks specific to their responsibilities. Try to avoid getting information out of objects. Try to avoid setting information into objects. When that is done, it will be much more clear what SRP is about.