Khi nào một người nên cố gắng loại bỏ một tuyên bố chuyển đổi?

Question

Tôi đã đi qua một tuyên bố chuyển đổi trong codebase tôi đang làm việc trên và tôi đang cố gắng tìm ra cách thay thế nó bằng một cái gì đó tốt hơn kể từ switch statements are considered a code smell. Tuy nhiên, khi đọc qua several các bài đăng trên stackoverflow về replacingswitchstatements Tôi dường như không thể nghĩ ra một cách hiệu quả để thay thế tuyên bố chuyển đổi cụ thể này.

Còn lại tôi tự hỏi liệu câu lệnh chuyển đổi cụ thể này có ổn không và nếu có trường hợp cụ thể trong đó câu lệnh chuyển đổi được coi là phù hợp.

Trong trường hợp của tôi mã (hơi obfuscated tự nhiên) mà tôi đang phải vật lộn với là như thế này:

private MyType DoSomething(IDataRecord reader) 
{ 
    var p = new MyType 
       { 
        Id = (int)reader[idIndex], 
        Name = (string)reader[nameIndex] 
       } 

    switch ((string) reader[discountTypeIndex]) 
    { 
     case "A": 
      p.DiscountType = DiscountType.Discountable; 
      break; 
     case "B": 
      p.DiscountType = DiscountType.Loss; 
      break; 
     case "O": 
      p.DiscountType = DiscountType.Other; 
      break; 
    } 

    return p; 
} 

bất cứ ai có thể đề xuất một cách để loại bỏ chuyển đổi này? Hay đây có phải là cách sử dụng thích hợp của công tắc không? Và nếu có, có sử dụng thích hợp nào khác cho các câu lệnh chuyển đổi không? Tôi thực sự muốn biết họ thích hợp ở đâu vì vậy tôi không lãng phí quá nhiều thời gian để loại bỏ mọi câu lệnh chuyển đổi mà tôi gặp phải chỉ vì chúng được coi là mùi trong một số trường hợp.

Cập nhật: Tại gợi ý của Michael Tôi đã làm một chút tìm kiếm sự trùng lặp của logic này và phát hiện ra rằng ai đó đã tạo ra logic trong một lớp học có hiệu quả đưa ra tuyên bố toàn bộ công tắc không cần thiết. Vì vậy, trong bối cảnh của mã bit cụ thể này, câu lệnh switch là không cần thiết. Tuy nhiên, câu hỏi của tôi là nhiều hơn về sự phù hợp của báo cáo chuyển đổi trong mã và cho dù chúng ta luôn luôn cố gắng thay thế chúng bất cứ khi nào chúng được tìm thấy như vậy trong trường hợp này, tôi nghiêng chấp nhận câu trả lời rằng câu lệnh chuyển đổi này là phù hợp.

Answer 1

Đây là cách sử dụng thích hợp cho một biểu tượng chuyển đổi, vì nó làm cho các lựa chọn có thể đọc được và dễ dàng thêm hoặc trừ một lựa chọn.

See this link.

Answer 2

tôi sẽ không sử dụng một if. An if sẽ ít rõ ràng hơn số switch. switch đang nói với tôi rằng bạn đang so sánh cùng một thứ trong suốt.

Chỉ cần để dọa người, đây là ít rõ ràng hơn so với mã của bạn:

if (string) reader[discountTypeIndex]) == "A") 
    p.DiscountType = DiscountType.Discountable; 
else if (string) reader[discountTypeIndex]) == "B") 
    p.DiscountType = DiscountType.Loss; 
else if (string) reader[discountTypeIndex]) == "O") 
    p.DiscountType = DiscountType.Other; 

switch Đây có thể là OK, bạn có thể muốn xem xét @Talljoe gợi ý.

Answer 3

Tuyên bố chuyển đổi này là tốt. Các bạn không có bất kỳ lỗi nào khác để tham dự không? lol

Tuy nhiên, có một điều tôi nhận thấy ... Bạn không nên sử dụng các chỉ số chỉ mục trên IReader [] đối tượng indexer .... nếu các lệnh cột thay đổi? Thử sử dụng tên trường tức là trình đọc ["id"] và trình đọc ["name"]

Answer 4

Các công tắc trên loại giảm giá có nằm trong mã của bạn không? Việc thêm loại giảm giá mới có yêu cầu bạn sửa đổi một số công tắc như vậy không? Nếu vậy, bạn nên xem xét việc bao thanh toán công tắc. Nếu không, sử dụng công tắc ở đây phải an toàn.

Nếu có nhiều chiết khấu cụ thể lây lan vi suốt chương trình của bạn, bạn có thể muốn cấu trúc lại như thế này:

p.Discount = DiscountFactory.Create(reader[discountTypeIndex]); 

Sau đó, đối tượng giảm giá có chứa tất cả các thuộc tính và phương pháp liên quan đến tìm ra chiết khấu.

Answer 5

Báo cáo chuyển đổi (đặc biệt là báo cáo dài) được coi là không tốt, không phải vì chúng là báo cáo chuyển đổi, mà vì sự hiện diện của chúng gợi ý cần phải tái cấu trúc.

Vấn đề với câu lệnh chuyển đổi là chúng tạo phân nhánh trong mã của bạn (giống như câu lệnh if). Mỗi nhánh phải được kiểm tra riêng rẽ, và mỗi nhánh trong mỗi nhánh và ... tốt, bạn có được ý tưởng.

Điều đó nói rằng, bài viết sau đây có một số thông lệ tốt về việc sử dụng báo cáo chuyển đổi:

http://elegantcode.com/2009/01/10/refactoring-a-switch-statement/

Trong trường hợp của mã của bạn, các bài viết trong diễn đàn cho thấy, nếu bạn đang thực hiện này loại chuyển đổi từ một kiểu liệt kê này sang kiểu liệt kê khác, bạn nên đặt công tắc của mình theo phương thức riêng của nó và sử dụng các câu lệnh trả về thay vì các câu lệnh ngắt. Tôi đã thực hiện điều này trước đây và mã trông sạch hơn nhiều:

private DiscountType GetDiscountType(string discount) 
{ 
    switch (discount) 
    { 
     case "A": return DiscountType.Discountable; 
     case "B": return DiscountType.Loss; 
     case "O": return DiscountType.Other; 
    } 
} 

Answer 6

Tôi nghĩ rằng việc thay đổi mã vì lợi ích của việc thay đổi mã không phải là thời gian sử dụng tốt nhất. Thay đổi mã để làm cho nó [dễ đọc hơn, nhanh hơn, hiệu quả hơn, v.v.] có ý nghĩa. Đừng thay đổi nó chỉ vì ai đó nói rằng bạn đang làm một cái gì đó 'có mùi'.

-Rick

Answer 7

Theo ý kiến ​​của tôi, nó không phải là câu lệnh chuyển đổi có mùi, đó là những gì bên trong chúng. Tuyên bố chuyển đổi này là ok, với tôi, cho đến khi nó bắt đầu thêm một vài trường hợp nữa. Sau đó, có thể đáng để tạo bảng tra cứu:

private static Dictionary<string, DiscountType> DiscountTypeLookup = 
    new Dictionary<string, DiscountType>(StringComparer.Ordinal) 
    { 
     {"A", DiscountType.Discountable}, 
     {"B", DiscountType.Loss}, 
     {"O", DiscountType.Other}, 
    }; 

Tùy thuộc vào quan điểm của bạn, điều này có thể ít nhiều có thể đọc được.

Nơi mọi thứ bắt đầu có mùi là nếu nội dung của vụ án của bạn nhiều hơn một hoặc hai dòng.

Answer 8

Tôi nghĩ rằng điều này phụ thuộc nếu bạn đang tạo MType thêm nhiều địa điểm khác nhau hoặc chỉ tại địa điểm này. Nếu bạn đang tạo MType ở nhiều nơi luôn phải chuyển sang loại dicsount có một số kiểm tra khác thì đây có thể là một mùi mã.

Tôi sẽ cố gắng tạo ra MTypes tại một điểm duy nhất trong chương trình của bạn có thể trong nhà xây dựng của chính MType hoặc trong một số phương pháp nhà máy nhưng có phần ngẫu nhiên trong chương trình của bạn. làm thế nào các giá trị nên và làm điều gì đó sai trái.

Vì vậy, việc chuyển đổi là tốt nhưng có lẽ việc chuyển đổi cần phải được di chuyển nhiều hơn bên trong phần tạo ra các loại của bạn

Answer 9

Vâng, điều này có vẻ như một cách sử dụng đúng của câu lệnh switch.

Tuy nhiên, tôi có một câu hỏi khác cho bạn.

Tại sao bạn chưa bao gồm nhãn mặc định?Việc ném một ngoại lệ trong nhãn mặc định sẽ đảm bảo rằng chương trình sẽ không đúng khi bạn thêm một DiscountTypeIndex mới và quên sửa đổi mã.

Ngoài ra, nếu bạn muốn ánh xạ giá trị chuỗi vào một Enum, bạn có thể sử dụng Thuộc tính và phản chiếu.

Cái gì như:

public enum DiscountType 
{ 
    None, 

    [Description("A")] 
    Discountable, 

    [Description("B")] 
    Loss, 

    [Description("O")] 
    Other 
} 

public GetDiscountType(string discountTypeIndex) 
{ 
    foreach(DiscountType type in Enum.GetValues(typeof(DiscountType)) 
    { 
     //Implementing GetDescription should be easy. Search on Google. 
     if(string.compare(discountTypeIndex, GetDescription(type))==0) 
      return type; 
    } 

    throw new ArgumentException("DiscountTypeIndex " + discountTypeIndex + " is not valid."); 
} 

Answer 10

Khi bạn thiết kế một ngôn ngữ và cuối cùng đã có một cơ hội để loại bỏ các, hầu hết các cú pháp dễ bị lỗi không trực quan xấu nhất trong cả ngôn ngữ.

RATNG khi bạn thử và xóa câu lệnh chuyển.

Chỉ cần rõ ràng, ý tôi là cú pháp. Đây là một cái gì đó được lấy từ C/C++ cần được thay đổi để phù hợp với cú pháp hiện đại hơn trong C#. Tôi hết lòng đồng ý với khái niệm cung cấp công tắc để trình biên dịch có thể tối ưu hóa bước nhảy.

Answer 11

Tôi không hoàn toàn phản đối các câu lệnh chuyển đổi, nhưng trong trường hợp bạn trình bày, tôi đã ít nhất đã loại bỏ sự trùng lặp của việc gán DiscountType; Thay vào đó, tôi có thể viết một hàm trả về một DiscountType cho một chuỗi. Hàm đó có thể đơn giản có các câu lệnh trả về cho mỗi trường hợp, loại bỏ sự cần thiết phải nghỉ ngơi. Tôi thấy sự cần thiết phải nghỉ giữa các trường hợp chuyển đổi rất nguy hiểm.

private MyType DoSomething(IDataRecord reader) 
{ 
    var p = new MyType 
       { 
        Id = (int)reader[idIndex], 
        Name = (string)reader[nameIndex] 
       } 

    p.DiscountType = FindDiscountType(reader[discountTypeIndex]); 

    return p; 
} 

private DiscountType FindDiscountType (string key) { 
    switch ((string) reader[discountTypeIndex]) 
    { 
     case "A": 
      return DiscountType.Discountable; 
     case "B": 
      return DiscountType.Loss; 
     case "O": 
      return DiscountType.Other; 
    } 
    // handle the default case as appropriate 
} 

Khá sớm, tôi đã nhận thấy rằng FindDiscountType() thực sự thuộc về lớp DiscountType và đã di chuyển hàm.

Answer 12

Robert Harvey và Talljoe đã cung cấp câu trả lời tuyệt vời - những gì bạn có ở đây là ánh xạ từ mã ký tự đến giá trị được liệt kê. Điều này được thể hiện rõ nhất như một bản đồ trong đó các chi tiết của ánh xạ được cung cấp ở một nơi, hoặc trong bản đồ (như Talljoe gợi ý) hoặc trong một hàm sử dụng câu lệnh chuyển đổi (như đề xuất của Robert Harvey).

Cả hai kỹ thuật đó có thể là tốt trong trường hợp này, nhưng tôi muốn thu hút sự chú ý của bạn đến một hiệu trưởng thiết kế có thể hữu ích ở đây hoặc trong các trường hợp tương tự khác. Các Mở/Đóng chính:

• http://en.wikipedia.org/wiki/Open/closed_principle
• http://www.objectmentor.com/resources/articles/ocp.pdf (! Chắc chắn rằng bạn đọc này)

Nếu ánh xạ là khả năng thay đổi theo thời gian, hoặc có thể được mở rộng thời gian chạy (ví dụ, thông qua một hệ thống plugin hoặc bằng cách đọc các phần của ánh xạ từ cơ sở dữ liệu), thì việc sử dụng Mẫu đăng ký sẽ giúp bạn tuân thủ nguyên tắc mở/đóng, có hiệu lực cho phép bản đồ được mở rộng mà không ảnh hưởng đến bất kỳ đồng nào de sử dụng ánh xạ (như họ nói - mở để mở rộng, đóng để sửa đổi).

Tôi nghĩ đây là một bài viết hay về Mẫu đăng ký - xem cách sổ đăng ký giữ ánh xạ từ một số khóa đến một số giá trị? Theo cách đó, nó tương tự như ánh xạ của bạn được biểu diễn như một câu lệnh switch.Tất nhiên, trong trường hợp của bạn, bạn sẽ không được đăng ký đối tượng mà tất cả thực hiện một giao diện chung, nhưng bạn sẽ nhận được các ý chính:

• http://sinnema313.wordpress.com/2009/03/01/the-registry-pattern/

Vì vậy, để trả lời câu hỏi ban đầu - báo cáo trường hợp là hình thức nghèo như tôi mong đợi các bản đồ từ mã ký tự đến một giá trị liệt kê sẽ là cần thiết ở nhiều nơi trong ứng dụng của bạn, do đó, nó phải được tính ra. Hai câu trả lời tôi tham khảo cho bạn lời khuyên tốt về cách thực hiện điều đó - hãy chọn lựa bạn thích. Tuy nhiên, nếu ánh xạ có khả năng thay đổi theo thời gian, hãy xem xét Mẫu đăng ký như là cách thức cách ly mã của bạn khỏi các tác động của thay đổi đó.

Answer 13

Bạn có quyền nghi ngờ câu lệnh chuyển đổi này: bất kỳ câu lệnh chuyển nào phụ thuộc vào loại nội dung nào đó có thể là dấu hiệu của sự đa hình bị mất (hoặc thiếu lớp con). Tuy nhiên,

Từ điển của TallJoe là một cách tiếp cận tốt.

Lưu ý rằng nếu giá trị enum và cơ sở dữ liệu là số nguyên thay vì chuỗi hoặc nếu giá trị cơ sở dữ liệu giống với tên enum, thì phản ánh sẽ hoạt động, ví dụ: trao

public enum DiscountType : int 
{ 
    Unknown = 0, 
    Discountable = 1, 
    Loss = 2, 
    Other = 3 
} 

sau đó

p.DiscountType = Enum.Parse(typeof(DiscountType), 
    (string)reader[discountTypeIndex])); 

sẽ đủ.