2012-04-05 18 views
5

Il metodo getCategory di seguito sembra molto ridondante e mi chiedevo se qualcuno avesse qualche suggerimento sul refactoring per renderlo più pulito possibilmente utilizzando un Enum. Sulla base del "val" passato, ho bisogno di getCategory per restituire l'istanza di categoria appropriata dalla classe Category. La classe Category è generata dal codice JNI, quindi non voglio cambiarlo. Qualcuno ha qualche idea?Refactoring del metodo Java con Enum

Metodo da riscritta:

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: Generated JNI (non può cambiare questo):

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, il tuo metodo è abbastanza pulito in questo momento, è un semplice metodo di fabbrica. A meno che non abbiate bisogno di un metodo come questo in molte altre classi, non vi suggerirò di dedicare del tempo a migliorare questo metodo. –

risposta

6

Il tuo metodo è essenzialmente mappatura da un predeterminato String a Category, quindi perché non utilizzare un Map invece? In particolare, io consiglierei di Guava di ImmutableMap, dal momento che questi mapping sono statici:

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

O il modo standard se non si desidera utilizzare una libreria di terze parti:

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

si potrebbe ancora impiegare il metodo per verificare la presenza di valori non validi (e sostenere caso-insensibilità come David Harkness ha sottolineato):

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

sull'utilizzo enumerazioni:

Se hai il controllo completo sui String s che sono passati in getCategory, e sarebbe solo essere di passaggio valori letterali, quindi ha senso per passare a un enum invece.

EDIT: precedenza, ho consigliato utilizzare un EnumMap per questo caso, ma Adrian's answer ha molto più senso.

+0

'getCategory' è case-insensitive. L'uso sopra descritto di "ImmutableMap" risponde a questo? –

+0

@Paul Bellora - Mi piace il tuo suggerimento, ma sto cercando una soluzione che utilizzi java 1.6. Credo che dovrei aggiungere un'altra libreria per implementare questo, corretto? – c12

+0

@DavidHarkness - Certo, hai ragione - quindi il metodo è ancora necessario, vedi la mia modifica. –

5

Se hai detto che vuoi fare il refactoring su enum, presumo tu intenda che non vuoi più passare la stringa per ottenereCategory per fare tutto questo lavoro. Il codice usa enum direttamente, invece di usare String.

Se questo è il caso, continuare a leggere

Per fortuna, vostra categoria è variabili statiche, in modo da poter fare qualcosa di reale straight-forward

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

Nel vostro vecchio codice, che si sta facendo qualcosa di simile :

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

Ora si sta facendo qualcosa di molto più ordinato

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

+1 Questo in realtà ha più senso della mia risposta sull'uso di enumerazioni. –

+0

@Adrian Shum - Devo ancora prendere decisioni basate su una stringa passata (ad esempio "produttore", "consumatore", ecc.). Quindi, se sto leggendo la tua risposta correttamente, avrei ancora la logica di commutazione if (val.equalsIgnoreCase ("producer"), corretta? – c12

+0

@ c12 - L'uso di un enum invece di stringhe letterali è sicuramente consigliato se possibile - ma se tu Sei costretto a basare le decisioni su un 'String' che deve essere passato, dovresti attenersi alla soluzione' Map'. –

1

Le risposte @PaulBellora e @AdrianShum sono entrambe ottime, ma penso che non si possa evitare di utilizzare il valore magico proprio come "produttore" (senza distinzione tra maiuscole e minuscole) per produrre Category per motivi di archiviazione. Quindi temo che il codice ridondante di getCategory non possa essere eliminato.(Purtroppo, i valori magici utilizzati per init Category e ottenere Category non sono uguali)

Ecco il codice per utilizzare Enum (Java 1.5 o superiore):

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

Se si utilizza lo stesso valore di magia per produrre, recuperare e conservare, è possibile:

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

e creare una Category come:

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

Ovviamente, è necessario modificare il codice per includere SampleJNI.CATEGORY_CONSUMER_get().