2012-04-05 13 views
5

El método getCategory a continuación parece muy redundante y me preguntaba si alguien tiene alguna sugerencia sobre la refacturación para que sea más limpio, posiblemente, utilizando un Enum. Basado en el "val" pasado, necesito getCategory para devolver la instancia de Categoría adecuada de la clase Category. La clase Category es un código JNI generado, por lo que no quiero cambiar eso. ¿Alguien tiene alguna idea?Refactorización del método Java utilizando Enum

Método que se refactorizado:

private Category getCategory(String val) throws Exception{ 
     Category category; 
     if (val.equalsIgnoreCase("producer")) { 
      usageCategory = Category.CATEGORY_PRODUCER; 
     } else if (val.equalsIgnoreCase("meter")) { 
      usageCategory = Category.CATEGORY_METER; 
     } else if (val.equalsIgnoreCase("consumer")) { 
      usageCategory = Category.CATEGORY_CONSUMER; 
     } else { 
      throw new Exception("Invalid value: " + val); 
     }  
     return usageCategory; 
    } 

Category.java: generada JNI (no puede cambiar esto):

public final class Category { 
    public final static Category CATEGORY_PRODUCER = new Category("CATEGORY_PRODUCER", SampleJNI.CATEGORY_PRODUCER_get()); 
    public final static Category CATEGORY_METER = new Category("CATEGORY_METER", SampleJNI.CATEGORY_METER_get()); 
    public final static Category CATEGORY_CONSUMER = new Category("CATEGORY_CONSUMER", SampleJNI.CATEGORY_CONSUMER_get()); 

} 
+0

IMO, su método es bastante limpio en este momento, es un método simple de fábrica. A menos que necesite un método como este en muchas otras clases, no le recomendaré que dedique tiempo a mejorar este método. –

Respuesta

6

Su método es esencialmente mapeo de una predeterminado String a Category, entonces ¿por qué no utilizar un Map? En concreto, me gustaría recomendar la guayaba de ImmutableMap, ya que estas asignaciones son estáticas:

private static final ImmutableMap<String, Category> CATEGORIES_BY_STRING = 
     ImmutableMap.of(
      "producer", Category.CATEGORY_PRODUCER, 
      "meter", Category. CATEGORY_METER, 
      "consumer", Category.CATEGORY_CONSUMER 
     ); 

O la forma estándar si no desea utilizar una biblioteca de terceros:

private static final Map<String, Category> CATEGORIES_BY_STRING; 
static { 
    Map<String, Category> backingMap = new HashMap<String, Category>(); 
    backingMap.put("producer", Category.CATEGORY_PRODUCER); 
    backingMap.put("meter", Category.CATEGORY_METER); 
    backingMap.put("producer", Category.CATEGORY_CONSUMER); 
    CATEGORIES_BY_STRING = Collections.unmodifiableMap(backingMap); 
} 

aún se podía emplear el método para comprobar si hay valores no válidos (y apoyar casos y la insensibilidad como David Harkness señaló):

private Category getCategory(String val) { 
    Category category = CATEGORIES_BY_STRING.get(val.toLowerCase()); 
    if (category == null) { 
     throw new IllegalArgumentException(); 
    } 
    return category; 
} 

sobre el uso de enumeraciones:

Si usted tiene un control completo sobre los String s que se pasan en getCategory, y sólo se pasan valores literales, entonces tiene sentido para cambiar a un enum lugar.

EDIT: Anteriormente, recomienda el uso de un EnumMap para este caso, pero Adrian's answer tiene mucho más sentido.

+0

'getCategory' no distingue entre mayúsculas y minúsculas. ¿El uso anterior de 'ImmutableMap' trata esto? –

+0

@Paul Bellora - Me gusta su sugerencia pero estoy buscando una solución que use java 1.6. Creo que tendría que agregar otra biblioteca para implementar esto, ¿correcto? – c12

+0

@DavidHarkness - Por supuesto, tienes razón, por lo que el método sigue siendo necesario, mira mi edición. –

5

Si dijo que desea refactorizar la base en enum, supongo que quiere decir que ya no desea pasar el String para que getCategory haga todo ese trabajo. El código está usando enum directamente, en lugar de usar String.

Si este es el caso, seguir leyendo

Por suerte, su categoría es variables estáticas, por lo que puede hacer algo real recta de avance

public enum FooCategory { // give a better name yourself 
    PRODUCER(Category.CATEGORY_PRODUCER), 
    METER(Category.CATEGORY_METER), 
    CONSUMER(Category.CATEGORY_CONSUMER) 

    private Category category; 
    FooCategory(Category category) { 
    this.category=category; 
    } 
    Category getCategory() { 
    return this.category; 
    } 
} 

En el código antiguo, que está haciendo algo parecido :

String fooCategory = "producer"; 
//.... 
Category category = getCategory(fooCategory); 
// work on category 

Ahora que está haciendo algo mucho más ordenado

FooCategory fooCategory = FooCategory.PRODUCER; 
//... 
Category category = fooCategory.getCategory(); 
// work on category 
+0

+1 Esto realmente tiene más sentido que mi respuesta sobre el uso de enumeraciones. –

+0

@Adrian Shum: aún necesito tomar decisiones basadas en un String pasado (es decir, "productor", "consumidor", etc.). Entonces, si estoy leyendo su respuesta correctamente, todavía tendría la lógica de cambio if (val.equalsIgnoreCase ("producer"), ¿correcto? – c12

+0

@ c12 - Si es posible, se recomienda utilizar una enumeración en lugar de literales de cadena, pero si 'se ven obligados a basar las decisiones en una' Cadena 'que se aprobó, tendrías que aferrarte a la solución' Mapa '. –

1

Las respuestas de @PaulBellora y @AdrianShum son geniales, pero creo que no se puede evitar usar el valor mágico como 'productor' (no distingue entre mayúsculas y minúsculas) para producir Category por razones de almacenamiento. Así que me temo que el código redundante en getCategory tampoco puede evitarlo.(Por desgracia, los valores mágicos utilizados para init Category y obtener Category no son iguales)

Éstos son el código para utilizar Enum (Java 1.5 o superior):

public enum Category { 
    CATEGORY_PRODUCER, 
    CATEGORY_METER, 
    CATEGORY_CONSUMER; 

    public static Category of(String val) throws Exception { 
     Category usageCategory; 
     if (val.equalsIgnoreCase("producer")) { 
      usageCategory = Category.CATEGORY_PRODUCER; 
     } else if (val.equalsIgnoreCase("meter")) { 
      usageCategory = Category.CATEGORY_METER; 
     } else if (val.equalsIgnoreCase("consumer")) { 
      usageCategory = Category.CATEGORY_CONSUMER; 
     } else { 
      throw new Exception("Invalid value: " + val); 
     }  
     return usageCategory; 
    } 
} 

Si se utiliza el mismo valor mágico a producir, ir a buscar y almacenar, puede:

public enum Category { 
    CATEGORY_PRODUCER("producer"), 
    CATEGORY_METER("meter"), 
    CATEGORY_CONSUMER("consumer"); 

    private String category; 
    private Category(String category) { 
     this.category = category; 
    } 

    public static Category of(String val) throws Exception { 
     Category usageCategory; 
     // You can use equals not equalsIgnoreCase, so you can use map to avoid redundant code. 
     // Because you use the same magic value everywhere. 
     if (val.equals("producer")) { 
      usageCategory = Category.CATEGORY_PRODUCER; 
     } else if (val.equals("meter")) { 
      usageCategory = Category.CATEGORY_METER; 
     } else if (val.equals("consumer")) { 
      usageCategory = Category.CATEGORY_CONSUMER; 
     } else { 
      throw new Exception("Invalid value: " + val); 
     }  
     return usageCategory; 
    } 
} 

y crear un Category como:

Category category = Category.of("producer"); 

Por supuesto, debe cambiar el código para incluir SampleJNI.CATEGORY_CONSUMER_get().