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);

"If they are not valid, I am returning null." - please read: http://stackoverflow.com/questions/1274792/is-returning-null-bad-design — Nir Alfasi, Dec 22 '16 at 08:16
For *working* code, codereview.stackexchange.com might be a better place. — GhostCat, Dec 22 '16 at 08:18
@GhostCat he didn't ask for a code-review, he pointed a specific problem with this implementation and ask how to solve it. It's fine to post it in code-review and it's okay to post it here as well since it deals with a specific programming issue. — Nir Alfasi, Dec 22 '16 at 08:30

Nir Alfasi · Accepted Answer · 2016-12-22T09:06:33.460

4

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!

edited Dec 22 '16 at 09:06

answered Dec 22 '16 at 08:28

Nir Alfasi

53,191
11
86
129

This looks quite good but I am little bit confuse. In my case, I am validating the records and sending back the `DataHolder` as well. So if records are valid, then only I will send DataHolder builder by populating otherwise I return null. In your example, how will I return my DataHolder if I split that way out? – john Dec 22 '16 at 08:33
@david since I'm not familiar with the requirements I can't answer this question. Returning `null` is a bad practice, I would prefer using `Optional< DataHolder>` as the returned type. I'll add it to the answer. Using an Optional is safer, and when you get the result you can safely check if `dataHolder.isPresent()` and act accordingly. – Nir Alfasi Dec 22 '16 at 08:37
In my case the requirement is, Given a record object, validate few fields on it by extracting them. And if they are valid, make a DataHolder object return it and if they are not valid for some reason, then return `Optional.absent();`. This is what I want to do mostly. And that's what I was doing but in a complicated way. – john Dec 22 '16 at 08:40
@david so you *do* use optional. Great! see the updated answer and also please re-read my last comment since I just edited it. – Nir Alfasi Dec 22 '16 at 08:41
I see.. I also have edited my question.. Added few things to add clarity how I was using it before. And how I will be using it now? Is that look right now? – john Dec 22 '16 at 08:48
@david yes it is clear. and as you can see "ProcessValidate" is a bad name and it breaks the single responsibility principle. What you probably want is to call it something like `DataHolderBuilder` which makes the validations and returns an `Optional` as described in the answer. You shouldn't need to call it twice (once for validations and second time to get the DataHolder), you should call it only once to get the `Optional` and let the class do its validations internally. – Nir Alfasi Dec 22 '16 at 09:00

score 2 · Answer 2 · answered Dec 22 '16 at 08:45

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.

That's a good point about naming. In this case also, my `fetchHolderWithChecks` method will still be clutted with all that code right? Do you think can we improve that if I want to return DataHolder from that method only? — john, Dec 22 '16 at 08:57
That is what my second part is about: when that method just iterates a list of validators and calls them ... then code becomes much simpler. You **definitely** do not want to have all those checks in one method. And as long as you have those checks within methods, and not objects, there is way to not call all those other check methods one by one. — GhostCat, Dec 22 '16 at 09:03
I see.. I have not used this pattern before. If you can provide an example how to use this, then it will help me to understand better because right now I am little bit confuse on how to use this within my code. — john, Dec 22 '16 at 09:05
As said; one way is: I) create that interface II) for each validation that you have in mind, implement that interface (could be a private inner class or so) III) instantiate objects of each impl class IV) iterate the list of those objects and call their check method — GhostCat, Dec 22 '16 at 09:48

score 1 · Answer 3 · answered Dec 22 '16 at 09:41

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.

How to write a method using single responsibility princpile?

3 Answers3