¿Cuándo se debe tratar de eliminar una declaración de cambio?

Question

He encontrado una declaración de cambio en la base de código en la que estoy trabajando y estoy tratando de encontrar la forma de reemplazarla por algo mejor desde switch statements are considered a code smell. Sin embargo, después de leer several mensajes en stackoverflow sobre replacingswitchstatements Parece que no puedo pensar en una forma efectiva de reemplazar esta declaración de cambio en particular.

Me quedé preguntándome si esta declaración de cambio en particular está bien y si hay circunstancias particulares donde las declaraciones de interruptor se consideran apropiadas.

En mi caso el código (un poco ofuscado, naturalmente) que estoy luchando con es así:

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; 
} 

¿Puede alguien sugerir una manera de eliminar este switch? ¿O es este un uso apropiado de un interruptor? Y si lo es, ¿hay otros usos apropiados para las declaraciones de cambio? Realmente me gustaría saber dónde son apropiados, así que no pierdo demasiado tiempo tratando de eliminar cada declaración de cambio que encuentro solo porque se consideran un olor en algunas circunstancias.

Actualización: A sugerencia de Michael lo hice un poco de búsqueda para la duplicación de esta lógica y descubrió que alguien había creado la lógica en otra clase que efectivamente hizo que toda la instrucción switch redundante. Entonces, en el contexto de este fragmento de código en particular, la declaración de cambio fue innecesaria. Sin embargo, mi pregunta es más acerca de lo apropiado de las declaraciones de cambio en el código y si siempre debemos tratar de reemplazarlas cada vez que se encuentran, por lo que en este caso me inclino a aceptar la respuesta de que esta declaración de cambio es apropiada.

Answer 1

Este es un uso apropiado para una declaración de cambio, ya que hace que las opciones sean legibles y fáciles de agregar o restar.

See this link.

Answer 2

yo no usaría un if. Un if sería menos claro que el switch. El switch me dice que estás comparando lo mismo en todas partes.

Sólo para asustar a la gente, esto es menos claro que su código:

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; 

Este switch puede estar bien, es posible que desee ver en @Talljoe sugerencia.

Answer 3

Esta instrucción de cambio es correcta. ¿Ustedes no tienen otros errores para atender? lol

Sin embargo, hay una cosa que noté ... No debería estar usando ordinales de índice en el indexador de objetos IReader [] .... ¿qué sucede si los pedidos de columna cambian? Intente usar los nombres de campo, es decir, reader ["id"] y reader ["name"]

Answer 4

¿Hay interruptores en el tipo de descuento en todo el código? ¿Agregar un nuevo tipo de descuento requiere que modifique varios de estos? Si es así, debes considerar factorizar el cambio. De lo contrario, usar un interruptor aquí debería ser seguro.

Si hay una gran cantidad de descuento propagación comportamiento específico a través de su programa, es posible que desee refactorizar esto como:

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

Entonces el objeto de descuento tiene todos los atributos y métodos relacionados con averiguar descuentos.

Answer 5

Las sentencias de cambio (especialmente las largas) se consideran malas, no porque sean declaraciones de conmutación, sino porque su presencia sugiere la necesidad de refactorizar.

El problema con las sentencias switch es que crean una bifurcación en el código (como lo hace una sentencia if). Cada rama debe probarse individualmente, y cada rama dentro de cada rama y ... bueno, se entiende la idea.

Dicho esto, el siguiente artículo tiene algunas buenas prácticas sobre el uso de sentencias switch:

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

En el caso de su código, el artículo en el enlace anterior sugiere que, si usted está realizando esta tipo de conversión de una enumeración a otra, debe poner su interruptor en su propio método y usar declaraciones de retorno en lugar de las declaraciones de interrupción. He hecho esto antes, y el código se ve mucho más limpio:

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

Answer 6

Creo que cambiar el código por el bien de cambio de código no es el mejor uso de los tiempos. Cambiar el código para hacerlo [más legible, más rápido, más eficiente, etc., etc.] tiene sentido. No lo cambies simplemente porque alguien dice que estás haciendo algo 'maloliente'.

-Rick

Answer 7

En mi opinión, no es cambiar las declaraciones que son el olor, que es lo que hay dentro de ellos. Esta declaración de cambio está bien, hasta que empiece a agregar un par de casos más. A continuación, puede valer la pena la creación de una tabla de búsqueda:

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

Dependiendo de su punto de vista, esto puede ser más o menos legible.

Donde las cosas empiezan a ponerse mal es si el contenido de su caso es más que una línea o dos.

Answer 8

Creo que esto depende de si está creando MType agregue muchos lugares diferentes o solo en este lugar. Si está creando MType en muchos lugares, siempre teniendo que cambiar para el tipo de dicount, tiene algunos otros controles, entonces esto podría ser un olor a código.

Intentaré crear MTypes en un solo lugar de tu programa, tal vez en el propio constructor del MType o en algún tipo de método de fábrica, pero tener partes aleatorias de tu programa podría hacer que alguien no lo sepa cómo deberían ser los valores y hacer algo mal.

Así que el interruptor es bueno, pero quizás el interruptor necesita ser movido dentro de la parte más creación de su tipo

Answer 9

Sí, esto parece un uso correcto de la sentencia switch.

Sin embargo, tengo otra pregunta para usted.

¿Por qué no ha incluido la etiqueta predeterminada?Al lanzar una excepción en la etiqueta predeterminada, se asegurará de que el programa fallará correctamente cuando agregue un nuevo descuentoTypeIndex y olvide modificar el código.

Además, si desea asignar un valor de cadena a un Enum, puede usar Atributos y reflexión.

Algo así como:

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

Al diseñar un lenguaje y finalmente tener la oportunidad de retirar el, más propenso a errores de sintaxis no intuitivo más feo de todo el lenguaje.

ESO es cuando intentas eliminar una declaración de cambio.

Para que quede claro, me refiero a la sintaxis. Esto es algo tomado de C/C++ que debería haberse cambiado para ajustarse a la sintaxis más moderna en C#. Estoy completamente de acuerdo con el concepto de proporcionar el interruptor para que el compilador pueda optimizar el salto.

Answer 11

No estoy absolutamente en contra de cambiar las declaraciones, pero en el caso que presente, al menos habría eliminado la duplicación de la asignación de DiscountType; En su lugar, podría haber escrito una función que devuelva un tipo de descuento dado una cadena. Esa función podría haber tenido simplemente las declaraciones de devolución para cada caso, eliminando la necesidad de un descanso. Considero que la necesidad de interrupciones entre los conmutadores es muy peligrosa.

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 
} 

Muy pronto, lo habría notado que FindDiscountType() realmente pertenece a la clase DiscountType y se trasladó a la función.

Answer 12

Robert Harvey y Talljoe han proporcionado excelentes respuestas: lo que tiene aquí es un mapeo desde un código de carácter hasta un valor enumerado. Esto se expresa mejor como un mapeo donde los detalles del mapeo se proporcionan en un lugar, ya sea en un mapa (como sugiere Talljoe) o en una función que usa una declaración de cambio (como lo sugiere Robert Harvey).

Ambas técnicas son probablemente buenas en este caso, pero me gustaría llamar su atención sobre un diseño principal que puede ser útil aquí o en otros casos similares. El Open/director cerrada:

• http://en.wikipedia.org/wiki/Open/closed_principle
• http://www.objectmentor.com/resources/articles/ocp.pdf (! Asegúrese de leer esto)

Si la asignación es probable que cambie con el tiempo, o posiblemente se extenderá tiempo de ejecución (por ejemplo, a través de un sistema de complemento o leyendo las partes del mapeo de una base de datos), entonces el uso del patrón de registro lo ayudará a cumplir con el principio abierto/cerrado, lo que permite extender el mapeo sin afectar a ningún co de eso usa el mapeo (como dicen, abierto para extensión, cerrado para modificación).

Creo que este es un buen artículo sobre el patrón de registro. ¿Ve cómo el registro contiene una asignación de alguna clave a algún valor? De esa manera, es similar a su mapeo expresado como una declaración de cambio.Por supuesto, en el caso de que no se le registro de objetos que toda implementar una interfaz común, pero debe obtener la esencia:

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

Por lo tanto, para responder a la pregunta original - la declaración de caso es deficiente, ya que espero que el mapeo del código de carácter a un valor enumerado sea necesario en varios lugares de la aplicación, por lo que debe tenerse en cuenta. Las dos respuestas a las que hice referencia le brindan buenos consejos sobre cómo hacer eso: elija cuál de ellas prefiere. Sin embargo, si el mapeo puede cambiar con el tiempo, considere el Patrón del Registro como una manera de aislar su código de los efectos de dicho cambio.

Answer 13

Tiene razón al sospechar esta declaración de cambio: cualquier instrucción de cambio que dependa del tipo de algo puede ser indicativa de polimorfismo faltante (o subclases faltantes).

El diccionario de TallJoe es un buen enfoque, sin embargo.

Tenga en cuenta que si sus valores de enumeración y de bases de datos eran números enteros en lugar de cadenas, o si sus valores de base de datos fueron los mismos que los nombres de enumeración, a continuación, la reflexión funcionaría, por ejemplo, dado

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

entonces sería suficiente

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

.