単一の責任を使用してメソッドを書く方法は？

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 = ... 

のように、リストを作成します。

このアプローチの素晴らしい点は、1つの専用クラス内で1種類の検証用のコードがあることです。検証を簡単に強化することができます。新しいimplクラスを作成するだけです。その既存のリストに対応するオブジェクトを追加します。

Answer 3

これは直接実行可能ではないかもしれませんが、このコードをクリーンアップする場合はまずオブジェクト指向（オブジェクト指向）を使用することをお勧めします。 OOを適切に使用していない場合は、SRPのように、OOの詳細を議論する必要はありません。

私が言っていることは、コードが約であることを伝えることができませんでした。あなたのクラス名は "ProcessValidate"（それは事でさえ？）、 "Record"、 "DataHolder"です。それはかなり疑わしいところです。

文字列リテラルは、あなたの識別子よりもドメイン（ "payId"、 "deviceId"、 "clientId"）の詳細を明らかにしますが、それは良い兆候ではありません。

あなたのコードは、物事を行うように依頼するのではなく、他の "オブジェクト"からデータを取り出すことに関するものです（オブジェクト指向の特徴）。

概要：ドメインを反映するオブジェクトにコードをリファクタリングしてみてください。これらのオブジェクトが責任に固有のタスクを実行するようにします。オブジェクトから情報を取得しないようにしてください。情報をオブジェクトに設定しないようにしてください。それが完了すると、SRPがどんなものであるかがはっきりします。