2017-06-08 18 views
1

したがって、あなたは単責任原則のために自分自身を教育しようとすると、「のような定義に遭遇する可能性が高い」というメッセージが表示されます。 "1クラスはを変更する理由の1つに過ぎません。"そして、そここの実際のコードで単一責任の原則を理解する

class Dog { 
    String bark() { 
     return "Woof"; 
    } 
} 

のようなおもちゃの例の束が常にあるしかし、私は、それは非常に難しいエンタープライズアプリケーションの開発にこの原則を適用するために見つけます。

"プロジェクト要素"、 "アクティビティ"、 "従業員"があるアプリケーションがあります。従業員には多くのアクティビティがあり、アクティビティには1つのプロジェクト要素があります。

一部の従業員はすべてのプロジェクト要素に割り当てられていますが、一部のプロジェクト要素には割り当てられているものもあります。プロジェクト要素をEmployeeから削除し、新しい要素を割り当てることができます。従業員は、自分に割り当てられたプロジェクト要素を持つ新しいアクティビティのみを追加できます。プロジェクト要素が割り当てられていない場合、これはすべてが利用可能であることを意味します。

私たちのクライアントには、新しいアクティビティを追加するときにお気に入りのプロジェクト要素の一覧が表示されることが必要でした。従業員がプロジェクト要素をどのようにお気に入りにするかは関係ありません。異なるビューです。

お気に入りのプロジェクト要素は、いくつかの興味深い規則で並べ替える必要があります。 これは、Favorite Project Elementが存在するアクティビティの合計計算作業に依存し、最後の60日間のみが考慮されます。少なくともそれは、具体的な要件です。..

だから、それはこれを実装するために私の義務だった、とここで私が持っている実装です:

public List<ProjectElement> getFavoriteProjectElementsSortedDescendingByTotalWorkHoursInLast60Days(Employee owner) { 
    // All Favorite Project Elements of Owner 
    final List<ProjectElement> favoriteProjectElementsOfOwner 
      = (List<ProjectElement>) (List<?>) favoriteBusinessObjectHelper.getAllFavoriteObjectsForBusinessObjectType(BusinessObjectType.PROJECT_ELEMENT.toInt()); 

    // Activities by Favorite Project Elements 
    final List<Activity> favoriteProjectElementActivitiesForLast60Days 
      = activityFinder.findByInProjectElementsForLastGivenDates(owner, favoriteProjectElementsOfOwner, 60); 

    // Create a Map with Favorite Project Element - Calculated Work 
    final Map<ProjectElement, Long> projectElementTotalWorkMap = new HashMap<ProjectElement, Long>(); 
    for (Activity activity : favoriteProjectElementActivitiesForLast60Days) { 
     final ProjectElement activityProjectElement = activity.getProjectElement(); 
     if (!projectElementTotalWorkMap.containsKey(activityProjectElement)) { 
      projectElementTotalWorkMap.put(activityProjectElement, 0L); 
     } 
     final long calculatedWork = activity.getCalculatedWork(); 
     final long totalCalculatedWork = projectElementTotalWorkMap.get(activityProjectElement) + calculatedWork; 
     projectElementTotalWorkMap.put(activityProjectElement, totalCalculatedWork); 
    } 

    // Sort the Map by value descending 
    final Map<ProjectElement, Long> sortedProjectElementTotalWorkMap 
      = InnboundSortTool.sortByValueDescending(projectElementTotalWorkMap); 

    // We do not want to show owners Favorite Project Element in the sidebar, if the Project Element is not available 
    // for Employee anymore.. See the comments at the end of the file. 
    final Set<ProjectElement> allowedProjectElementsForOwner = owner.getExplicitlyAssignedOnlyActiveProjectElements(); 

    final ArrayList<ProjectElement> projectElementsSorted = new ArrayList<ProjectElement>(); 
    for (ProjectElement projectElement : sortedProjectElementTotalWorkMap.keySet()) { 
     if (allowedProjectElementsForOwner.size() == 0) { // This means Employee does not have any restrictions, all Project Elements are available to him. 
      projectElementsSorted.add(projectElement); 
     } else { // If Employee has assigned Project Elements, we must check if the Favorite Project Element is assigned to him.. 
      if (allowedProjectElementsForOwner.contains(projectElement)) { 
       projectElementsSorted.add(projectElement); // If yes, add it to list, if no simply continue the loop without adding. 
      } 
     } 
     if (projectElementsSorted.size() == 20) { 
      break; // 20 is an arbitrary value, we do not want to show too many Favorite Project Elements in the UI.. Limit by 20. 
     } 
    } 

    return projectElementsSorted; 
} 

おっと、それは一つの大きな方法ですが、それは仕事を取得します完了しました。しかし、それは一つのことをしませんか?しかし、すべての方法が1つのことだけを行う場合、誰がそのすべてをやろうとしていますか?

私はヘルパークラスを導入し、そのクラスにすべてを委任し、呼び出しを開始ください:

final Map<ProjectElement, Long> projectElementTotalWorkMap = helper.CreateprojectElementTotalWorkMap(); 

helper.removeUnassignedFavoriteProjectElementsFromEmployee(); 

など?しかし、ヘルパー自体にヘルパーを導入するのですか?それはどこで終了しますか?私はこのようなリファクタリングを開始すると、私は他のメソッドを呼び出す非常に役に立たない方法を、持ってしまうと、このクラスは次のようになります。

List<ProjectElement> favoritesList; 
favoritesList = helper.doThis(); 
favoritesList = helper.doThat(); 
favoritesList = helper.sort(); 
return favoritesList; 

私はすべてこの原則を理解しないのですか?私はそうではないと思うので、ここに質問があります。このメソッドを "SRP"に従うように修正する方法はありますか?

答えて

1

私が心に留めておくべき重要なことは、あなたの責任を同じ抽象化レベルに保つことだと思います。意味することは、この1つの機能をいくつかに分割すると、多くのことが実行されますが、より低いレベルの抽象化でオーケストレーションを行うという責任が1つあります。

public List<ProjectElement> getFavoriteProjectElementsSortedDescendingByTotalWorkHoursInLast60Days(Employee owner) { 
    // All Favorite Project Elements of Owner 
    final List<ProjectElement> favoriteProjectElementsOfOwner 
      = (List<ProjectElement>) (List<?>) favoriteBusinessObjectHelper.getAllFavoriteObjectsForBusinessObjectType(BusinessObjectType.PROJECT_ELEMENT.toInt()); 

    // Activities by Favorite Project Elements 
    final List<Activity> favoriteProjectElementActivitiesForLast60Days 
      = activityFinder.findByInProjectElementsForLastGivenDates(owner, favoriteProjectElementsOfOwner, 60); 

    // Create a Map with Favorite Project Element - Calculated Work 
    final Map<ProjectElement, Long> projectElementTotalWorkMap = this.makeFacoriteProjectMap(favoriteProjectElementActivitiesForLast60Days) 

あなたprojectElementTotalWorkMapを生成機能は、プライベート関数に移動されました。ここでは例として、

は、いくつかのリファクタリング後のあなたの関数の最初の部分です。だから、マップを構築する方法を理解することは、もはやあなたの主な責任ではありません。あなたは、あなたはこのマップを生成する方法でエラーを発見した場合、

private Map<ProjectElement, Long> projectElementTotalWorkMap makeFacoriteProjectMap(List<Activity> favoriteProjectElementActivitiesForLast60Days) 
{ 
    Map<ProjectElement, Long> projectElementTotalWorkMap= new HashMap<ProjectElement, Long>(); 
    for (Activity activity : favoriteProjectElementActivitiesForLast60Days) { 
     final ProjectElement activityProjectElement = activity.getProjectElement(); 
     if (!projectElementTotalWorkMap.containsKey(activityProjectElement)) { 
      projectElementTotalWorkMap.put(activityProjectElement, 0L); 
     } 
     final long calculatedWork = activity.getCalculatedWork(); 
     final long totalCalculatedWork = projectElementTotalWorkMap.get(activityProjectElement) + calculatedWork; 
     projectElementTotalWorkMap.put(activityProjectElement, totalCalculatedWork); 
    } 

    return projectElementTotalWorkMap; 
} 

以前:方法に誤りがマップが生成されていることがある場合、あなたはあなたがそれを動かす機能で見てする必要があります主な機能を変えなければならなかったでしょう。例えば、事態が間違った順序で起こった場合など、主な機能を変更する必要があります。

オーケストレーションは現在、主な機能のみの責任です。

この同じリファクタを使用してprojectElementsSorted ArrayListを生成した後、関数getFavoriteProjectElementsSortedDescendingByTotalWorkHoursInLast60Daysは1つの責任を負うことになります。最後の60日間の作業時間のソートされたリストを提供する手順を調整します。

それぞれ独自の責任を持つ他の小さなプライベート機能があります。それ自身のことをしている多くのプライベート関数があっても、このコードの主なクラスは依然として外界に対して1つの責任を負います。

あなたの犬の鳴き声の例と結びつけて、別の類推をしましょう。サーカスでルーチンを実行するためにあなたの犬を訓練するトレーナーがいるとしましょう。一日の終わりには犬がこれを行うことができる必要があります。この機能は、多くのことを行い

class Dog 
{ 
    public void PerformRoutine() 
    { 
     this.bark(); 
     this.sit(); 
     this.walkInCircle(); 
     this.rollOver(); 
    } 
    //... 
} 

にもかかわらず、今までに変更するには、この機能のための唯一の理由は、ルーチン変更された場合になります。しかし、鳴き声が変わる必要がある場合は、犬がここを見ません。 PerformRoutine関数内のすべての関数は、同じ抽象レベルにあります。あなたは、多くの場合、ドメインオブジェクトの保存と読み込み、呼び出しを編成サービス/ API層を持つエンタープライズアプリケーションのコンテキストで

。これらの機能はSRに違反しているように見えますが、これらはすべて同じ責任の一部です。保存と読み込みはリポジトリの責任です。実際のロジックは、ドメインオブジェクトの責任です。これらのAPI /サービスは、実際には1つのことを行います。つまり、他のサービスを調整して委任します。

こちらがお役に立てば幸いです。

EDIT:このすべての値が何であるかにあなたの質問に答えるために:

SRせずに前の犬の例を考えてみましょう:

class Dog 
{ 
    public IEnumerator PerformRoutine() 
    { 
     this.audioController.PlaySound("c:/bar.mp3"); 
     this.animationControl.Play("Sit.anim"); 

     yield return new WaitForSeconds(2); 

     this.animationController.Play("stand.anim");   

     yield return new WaitForSeconds(1); 

     foreach(Vector3 pos in this.WalkPositions) 
     { 
      this.animationController.Play("walk.anim"); 
      this.position.lerpTo(pos, 5.0f); // move to position over 5 seconds. 

     } 

     this.animationController.Play("roll.anim"); 
    } 
    //... 
} 

すべての新しいチームメンバーは、今まさに理解する必要がありますが実際に何を理解するためにこの関数の実装の詳細。これは既に大幅に簡素化されています。

あなたが(または、あなたの古いコードを再訪しても)新しいチームメンバーを依頼する場合は、理想的にはその上に一目にしたいし、それが何をするのかを理解するであろう。関数名 "PerformRoutine"は、リーダにアニメーションまたは動作ロジックを準備するものではありません。

犬が円ではなく正方形を歩くので、このコードに戻ってきたら、このコードを調整するのが難しくなります。

この同じ理由を、数十万行のコードと何千もの関数を持つエンタープライズソリューションに適用します。最終的には終了したくないほど管理が難しくなります。

ここでは、投資会社向けに行ったREAL LIFE統合の別の例を示します。おおよそ私の開発スケジュールの数日を要した第三者コードです:

public List<Event> getEventData() 
{ 
    var toReturn = new List<Event>(this.eventData); 
    this.eventData.Clear(); 
    return toReturn; 
} 

ご覧ください。この関数を2回連続して呼び出すと、作者がSRまたは適切な命名を気にしなかったため、大幅に異なる出力が得られます。

最小限の労力で、これは2つの機能、つまりgetEventDate()clearEventData()でした。後者はSRに違反しますが、私は別の名前を持つ1つの関数 - getAndClearEventData()で解決しました。

+0

はい、間違いなく、ありがとうございました。しかし、まだ、5行のコードをプライベートメソッドに移動するだけですか?おそらく私はそれに付随する概念(または優位性)を理解することができない人です。 –

+0

@KorayTugayあなたはそれが無意味で余分な仕事のように見えることは間違いありません。しかし、私のチームのメンバーがコードを理解する必要があるときに、私のためのこの力が出てきます。大きな画像を理解する必要がある場合、これらの「オーケストレーション」機能を見ることができます。物事がうまくいかないときにだけ、彼らはもっと深く進む必要があり、うまくいけば彼らはどこを見なければならないのか理解できるようになります。それはまた、大きな機能を見るために非常に威圧的で認知的な過負荷です。良い読み物は、ロバートマーティンのクリーンコードです。 – Reasurria

関連する問題