단일 책임 원칙을 사용하여 방법을 작성하는 방법은 무엇입니까?

Question

isValid 메서드가 호출되는 아래 클래스가 있습니다.

• isValid 메서드에서 Record 개체에서 몇 가지를 추출하려고합니다. 그리고 난 그 분야의 일부를 검증하고 있습니다. 유효하다면 일부 추가 필드가있는 holder 맵을 채우고 내 DataHolder 빌더 클래스를 채우고 마지막으로 DataHolder 클래스를 반환합니다.
• 유효하지 않은 경우 null을 반환합니다.

  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 방법 : 아래

내 클래스? 여러 부분으로 나눌 수 있습니까? 아니면 그 코드를 작성하는 더 좋은 방법이 있습니까?

또한 레코드가 유효하지 않은 경우 null을 반환하는 다른 else 블록에있는 코드가 마음에 들지 않습니다. 나는 그것이 훨씬 더 나은 방법으로 쓰여질 수 있다고 확신한다.

업데이트 : 내 경우

이 내가 무엇을하고 있었는지입니다. 나는 이런 식으로 호출 오전 :

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라는 이름의 방법은()이 부울 값을 반환해야한다는 것입니다. DataHolder를 반환하는 것을 정말로 고려한다면; 내 제안이 같은 이름 (과 의미 약간)를 변경하는 것입니다 :

DataHolder fetchHolderWithChecks(String processName, Record ...) 

을 그리고 난 널 (null)을 반환하지 것 - 선택 사양 중 하나; 또는 단순히 예외를 던집니다. 사용자에게 발생한 오류에 대해 사용자에게 알리고 싶지 않으십니까? 그래서 예외를 던지면 더 높은 수준으로 오류 메시지를 제공 할 수 있습니다.검증 자체에

: 그 인터페이스의

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

그리고 다양한 구현 : 나는 종종이 같은 것을 사용합니다.

그리고는은 "검증 진입 점은"어떻게 든 같은 목록을 만들 것입니다

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

에 마지막으로 으로 반복 하나 이러한 측면에서 하나의 유효성을 검사 목록.

좋은 접근법 : 하나의 전용 클래스 내에서 한 종류의 유효성 검사를위한 코드가 있습니다. 검증을 쉽게 향상시킬 수 있습니다. 새로운 impl 클래스를 생성하는 것만으로; 해당 객체를 해당 기존 목록에 추가하는 것입니다.

Answer 3

이 코드는 직접 실행 가능하지 않을 수도 있지만이 코드를 정리하려면 먼저 OO (Object-Orientation)를 사용하는 것이 좋습니다. OO를 제대로 사용하지 않는다면 SRP와 같이 OO에 대한 세부 정보를 언급 할 필요가 없습니다.

내 말은 코드가 약 인 것을 말할 수 없다는 것입니다. 귀하의 클래스 이름은 "ProcessValidate"(심지어 그 것입니까?), "Record", "DataHolder"입니다. 바로 거기에서 꽤 의심 스럽습니다.

문자열 리터럴은 식별자보다 도메인 ("payId", "deviceId", "clientId")에 대해 더 많은 것을 보여줍니다. 이는 좋은 징조가 아닙니다.

귀하의 코드는 물건 (OO의 각인)을 수행하는 대신 다른 "객체"에서 데이터를 가져 오는 것입니다.

요약 : 코드를 도메인을 반영하는 개체로 리팩터링하십시오. 이 오브젝트가 자신의 책임에 맞는 태스크를 수행하게하십시오. 객체에서 정보를 얻지 않도록하십시오. 정보를 객체로 설정하는 것을 피하십시오. 이것이 끝나면 SRP가 무엇인지 명확하게 알 수 있습니다.