2014-04-16 1 views
2

일부 오래된 코드를 업그레이드하는 동안이 두 가지 OO 원칙이 서로 충돌하는 상황을 발견했습니다.우선 순위 : 자신 또는 단일 책임 원칙을 반복하지 마십시오?

는 (그것이 내가 발견 한 내용의 단순화 된 버전이다) 다음의 의사를 고려

int numberOfNewRecords; 
int numberOfOldRecords; 
int numberOfUndefinedRecords; 

void ColourAndCount() 
{ 
    foreach(Row row in dataGridView1.Rows) 
    { 
     switch(row.Cells["Status"]) 
     { 
      case: "Old" 
       row.BackColor = Color.Red; 
       numberOfOldRecords++; 
       break; 
      case: "New" 
       row.BackColor = Color.Blue; 
       numberOfNewRecords++; 
       break; 
      default: 
       row.BackColor = Color.White; 
       numberOfUndefinedRecords++; 
       break; 
     } 
    } 
} 

이 코드는 두 가지 작업을 수행 : 그것은 집계 상태로 레코드의 수까지, 그리고 그것은 또한 각각 colourises 행으로, 다시 그들의 상태. 지저분하지만이 두 작업이 항상 동시에 호출 되었기 때문에 문제가 발생하지 않았으며 추가 상태와 같은 유지 관리 요구 사항을 쉽게 추가 할 수있었습니다.

(편집) 마이너 참고 :

그럼에도 불구하고, 원리 나에게 말한다 단일 책임은 내가 두 개의 별도의 방법으로이 분할해야 난 그냥 용어를 여기에 "단일 책임 원칙"을 오용 할 수있다 깨달았다있는 나는 그것이 수업을 말하는 것을 이해한다. "메소드 당 하나의 연산"디자인 패턴의 용어는 무엇입니까?

int numberOfNewRecords; 
int numberOfOldRecords; 
int numberOfUndefinedRecords; 

void Count() 
{ 
    foreach(Row row in dataGridView1.Rows) 
    { 
     switch(row.Cells["Status"]) 
     { 
      case: "Old" 
       numberOfOldRecords++; 
       break; 
      case: "New" 
       numberOfNewRecords++; 
       break; 
      default: 
       numberOfUndefinedRecords++; 
       break; 
     } 
    } 
} 

void Colour() 
{ 
    foreach(Row row in dataGridView1.Rows) 
    { 
     switch(row.Cells["Status"]) 
     { 
      case: "Old" 
       row.BackColor = Color.Red; 
       break; 
      case: "New" 
       row.BackColor = Color.Blue; 
       break; 
      default: 
       row.BackColor = Color.White; 
       break; 
     } 
    } 
} 

하지만이 자신을 반복하지 마십시오 위반 : 루프와 스위치 문이 두 방법에 복제하고,이 코드에 대한 가장 가능성있는 업그레이드 경로부터 다른 상태 중 추가입니다, 그것은 향후 업그레이드가 더 어려워 보다 적게보다는 오히려.

리팩터링하는 가장 우아한 방법을 찾는 데 어려움을 겪었으므로 놓친 부분이있는 경우 커뮤니티에 물어 보는 것이 가장 좋습니다. 이 상황을 어떻게 처리하겠습니까?

(EDIT)

나는 하나 개의 가능한 솔루션을 함께했다하지만, 간단한 문제 엔지니어링을 통해-의 예처럼 내게 보이는 (그리고 정말 원래 하나의 책임 문제가 해결되지 않습니다).

struct Status 
{ 
    public string Name, 
    public int Count, 
    public Color Colour, 
} 

Dictionary<string, Status> StatiiDictionary = new Dictionary<string, int>(); 
void Initialise() 
{ 
    StatiiDictionary.Add(new Status("New", 0, Color.Red)); 
    StatiiDictionary.Add(new Status("Old", 0, Color.Blue)); 
    StatiiDictionary.Add(new Status("Undefined", 0, Color.White)); 
} 

void ColourAndCountAllRows() 
{ 
    foreach(Row row in dataGridView1.Rows) 
    { 
     CountRow(row, StatiiDictionary); 
     ColourRow(row, StatiiDictionary); 
    } 
} 

void CountRow(Row row, Dictionary<string, Status> StatiiDictionary) 
{ 
    StatiiDictionary[row.Cells["Status"]].Count++; 
} 

void ColourRow(Row row, Dictionary<string, Status> StatiiDictionary) 
{ 
    row.BackColour = StatiiDictionary[row.Cells["Status"]].Colour; 
} 
+0

원래 코드와 비슷한 코드가 단일 위치에서 사용되는 한 루프 내에서 스위치를 수행하는 것이 허용 될 수 있습니다. 그러나 유형별로 선택한 행을 계산하는 것과 같은 다른 연산을 추가 할 수 있다면 Matt 대답의 아이디어를 사용하여 코드를 리팩터링하는 것이 좋습니다. – Phil1970

답변

5

실용적인 이유로 인해 수시로 위반해야하는 프로그래밍 규칙이 많이 있습니다.

  1. 그것은 각각 순종하는 코드 무엇을 의미 : 귀하의 경우에, 나는 DRY 및 SRP 그래서 난 승자를 결정하는 두 가지 기준을 제안 경쟁을 한 것으로 나타났습니다 동의 것입니다.
  2. 응용 프로그램이 각 응용 프로그램에 순종한다는 것은 무엇을 의미합니까? 이 경우

는 그리드의 행을 열거의 비 효율성은 두 번이 특별한 경우에 을 이길 것 나와 DRY에 무시 요인이 될 것으로 보인다. 다른 경우에는 다른 방향 일 수 있습니다.

결정을 설명하는 이유와 그 이유를 나중에 코드를 보는 사람에게 분명히 알리는 것이 좋습니다. 이는 정확하게 코멘트가 사용되어야하는 것과 같습니다. 즉, 코드가 수행중인 작업보다 수행중인 작업을 수행하는 이유입니다.

4

디자인 패턴은 의사 ​​결정에 영향을줍니다. 그들은 종교적으로 따르지 않아야합니다. 코드를 유지 관리하기 쉽도록 속성을 추가 할 수 있습니다. 모든 디자인 패턴에 따라 총체적인 풍성화가이되며 이는 특정 원칙을 무시하는 것보다 훨씬 나쁩니다.

여러 가지 연산을 하나의 반복자로 결합하는 것이 합당한 것처럼 보입니다. 이것은 두 번 반복하는 것보다 효율적이지만 두 가지 작업 모두를 수행하는 것을 제한합니다. 따라서 실행 가능한 구현의 결과 속성을 사용하여 가장 적합한 판단을합니다. 작업을 개별적으로 적용하는 것이 중요하다고 생각하면 자신을 반복하는 것이 좋습니다.

두 가지 모두를 만족시킬 수있는 해결책이 있지만 문제는 매우 과장 될 수 있다는 것입니다. 속성은 당신이 당신의 코드는 최초의 솔루션의 효율성의 혜택을 모두 DRY 및 SRP 존중해야 할 수 있습니다

  • 은 개별적으로 작업에 적용 할 수 있도록 허용 - SRP
  • 을 행에 걸쳐 반복 반복하지 두 번 - DRY/효율성
  • 가 두 번 같은 switch 문을 반복하지 - DRY/효율성

을 유사한 의사에 대신 기능을 하나의 자바 스타일의 접근 방식을 사용하여 다음과 같은 솔루션이 기준을 충족 할 수 있습니다

public abstract class RowOperation { 
    public void apply(string status, Row row) { 
     switch(status) 
     { 
      case: "Old" 
       this.OldCase(row); 
       break; 
      case: "New" 
       this.NewCase(row); 
       break; 
      default: 
       this.OtherCase(row); 
       break; 
     } 
    } 
    abstract void OldCase(Row); 
    abstract void NewCase(Row); 
    abstract void OtherCase(Row); 
} 

public class ColorRow implements RowOperation { 
    private static final ColorCells OP = new ColorCells(); 

    private ColorCells(){} 

    // This operation isn't stateful so we use a singleton :D 
    public static RowOperation getInstance() { 
     return this.OP 
    } 

    public void OldCase(row) { 
     row.BackColor = Color.Red; 
    } 

    public void NewCase(row) { 
     row.BackColor = Color.Blue; 
    } 

    public void OtherCase(row) { 
     row.BackColor = Color.White; 
    } 
} 

public class CountRow implements CellOperation { 
    public int oldRows = 0; 
    public int newRows = 0; 
    public int otherRows= 0; 

    // This operation is stateful so we use the contructor 
    public CountRow() {} 

    public void OldCase(Row row) { 
     oldRows++; 
    } 

    public void NewCase(Row row) { 
     newRows++; 
    } 

    public void OtherCase(Row row) { 
     otherRows++; 
    } 
} 

// For each row in the grid we will call each operation 
// function with the row status and the row 
void UpdateRows(Grid grid, RowOperation[] operations) 
{ 
    foreach(Row row in grid.Rows) 
    { 
     string status = row.Cells["Status"] 

     foreach(RowOperation op in operations) 
     { 
      op.apply(status, row) 
     } 
    } 
} 

그리고 당신이 그 (것)들에게

RowOperations[] ops = { 
    ColorRow.getInstance(), 
    new CountRows()  
}; 

UpdateRows(dataGrid1, ops); 

을 필요로하는 당신은 새로운 작업을 추가 한 반복의 행에 여러 작전을 적용 할 수 있습니다하지만 당신은 알 수 있습니다로 모든 디자인 패턴을 구현하는 당신이 리드에 대해 읽고 이 과도한 해결책으로 나는 심지어 클래스 계층 구조의 수준을 건너 뛴다. 그리고 그것은 여전히 ​​꽤 나쁘다. 이 코드는 여기에서 존중되는 디자인 패턴의 모든 이점을 가지고 있지만, 응용 프로그램의 맥락에서 질문은 정말로 이러한 모든 속성이 필요합니까? 대답은 아마도 아니오입니다.

+0

당신의 "설계된"코드에는 많은 좋은 아이디어가 있지만 각각의 코드가 단순하다면 대신에 많은 코드를 쓸 필요가 없기 때문에'Action '을 사용할 수 있습니다. – Phil1970

+0

당신이 코드를 싫어하는 이유는 여러 작업을 수행 할 때마다 매번 디스패치를 ​​반복한다는 것입니다. 이것은 복합 연산이 의미가있는 곳입니다. 예를 들어, 복합체의'OldCase' 메쏘드는리스트의 각 항목의'OldCase' 메쏘드를 호출합니다. – Phil1970

관련 문제