如何使用单一职责原则编写方法？

Question

我有下面的 class，其中正在调用 isValid 方法。

我试图从 isValid 方法中的 Record 对象中提取一些东西。然后我验证了其中的几个字段。如果它们有效，那么我将使用一些附加字段填充 holder 地图，然后填充我的 DataHolder 构建器 class，最后 return DataHolder class回来了。
如果它们无效，我将return设为空。

下面是我的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;
  }
}

我的 isValid 方法做很多事情吗？能否分解成多个部分？或者有没有更好的方法来编写该代码？

此外，我对 else 块中的代码感觉不太好，如果记录无效，我 return null。我很确定它可以写得更好。

更新：

就我而言，这就是我所做的。我这样称呼它：

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

所以现在这意味着我必须这样做：

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

你是对的 isValid() 做的事情太多了。但不仅如此，当我们大多数人看到一个名为 isValid() 的方法时——我们期望返回一个布尔值。在这种情况下，我们返回 DataHolder 的实例，这是违反直觉的。

尝试拆分你在方法中做的事情，例如：

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

然后用不同的方法构造DataHolder，比如说：

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

它将使您的程序更易于阅读和维护！

Answer 2

我认为事情从这里的命名开始。

正如 alfasin 正确指出的那样，非正式约定是名为 isValid() 的方法应该 return a boolean 价值。如果你真的考虑 returning 一个 DataHolder；我的建议是更改名称（和语义），像这样：

DataHolder fetchHolderWithChecks(String processName, Record ...)

而且我不会 return null - 要么是可选的；或者只是抛出一个异常。你看，难道你不想告诉你的用户发生的那个错误吗？因此，当抛出异常时，您将有办法向更高级别提供错误消息。

关于验证本身：我经常使用这样的东西：

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

然后是该接口的各种实现。

然后，"validation entry point" 会以某种方式创建一个列表，例如

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

最终迭代该列表以逐一验证这些方面。

这种方法的好处是：您在一个专用 class 中拥有用于一种验证的代码；你可以很容易地增强你的验证；只需创建一个新的 impl class;并将相应的对象添加到该现有列表中。

Answer 3

我知道这可能无法直接操作，但如果你想清理这段代码，你应该做的第一件事就是使用 OO（面向对象）。如果您没有正确使用 OO，那么就没有必要争论 OO 的更精细的细节，例如 SRP。

我的意思是，我无法判断您的代码是 about。您的类名是 "ProcessValidate"（这甚至是一回事吗？）、"Record"、"DataHolder"。那里很可疑。

字符串文字比您的标识符更能揭示域（"payId"、"deviceId"、"clientId"），这不是一个好兆头。

您的代码完全是关于从其他 "objects" 获取数据，而不是要求他们做一些事情（OO 的标志）。

总结：尝试将代码重构为反映您领域的对象。让这些对象执行特定于其职责的任务。尽量避免从对象中获取信息。尽量避免将信息设置到对象中。完成后，SRP 是什么就更清楚了。

如何使用单一职责原则编写方法？

How to write a method using single responsibility princpile?

java

oop

design-patterns

single-responsibility-principle

guava