1

I’m working on a Java Swing unit converter. The user selects a unit category from a JComboBox, and based on that selection, I update two other JComboBox<Units> with corresponding enum values (Length.values(), Weight.values(), etc.).

Currently, my method looks like this:

private void selectUnit(ActionEvent e) {

        if (view.getCategoryBox().getSelectedItem().toString().equals("LENGTH")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Length.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Length.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("WEIGHT")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Weight.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Weight.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("TEMPERATURE")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Temperature.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Temperature.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("TIME")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Time.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Time.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("VOLUME")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Volume.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Volume.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("AREA")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Area.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Area.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("SPEED")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Speed.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Speed.values()));
        } else if (view.getCategoryBox().getSelectedItem().toString().equals("ENERGY")) {
            view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(Energy.values()));
            view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(Energy.values()));
        }
    }

This works, but it’s repetitive and feels hard to maintain.

Is there a cleaner or more elegant way to refactor this without using so many if/else statements?

4
  • 2
    Try a switch statment. Commented May 9 at 18:37
  • 1
    Use a HashTable to map the selected item to your enums. Something like: stackoverflow.com/a/23205712/131872 Commented May 9 at 19:13
  • If your code works, but you want to improve it, consider if it would be better here or at Code Review. Be sure and compare Stack Overflow's and Code Review's requirements before asking. Commented May 9 at 20:41
  • See also the Converter example, cited here and here. Commented May 13 at 11:58

2 Answers 2

3

Separate your concerns.

  1. You have a String and want to know which enum it corresponds to.
  2. You (now) have an enum, and want to extract its values and pass them along.

For #1, I would do something like this.

    final Class<?> clazz =
        switch (value)
        {

            case "LENGTH"      -> Length.class;
            case "WEIGHT"      -> Weight.class;
            case "TEMPERATURE" -> Temperature.class;
            case "TIME"        -> Time.class;
            case "VOLUME"      -> Volume.class;
            case "AREA"        -> Area.class;
            case "SPEED"       -> Speed.class;
            case "ENERGY"      -> Energy.class;
            default            -> null;

        }
        ;

Ok, now you have a switch expression that takes in a String and then gives you a Class for an enum.

Ok, now let's do #2 and turn that Class into the array of values. This is also easy.

//we have the variable clazz from above
if (clazz != null)
{
    view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(clazz.getEnumConstants()));
    view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(clazz.getEnumConstants()));
}

Note -- the method clazz.getEnumConstants() returns the exact same thing as calling values() would for the enum that the variable clazz is pointing to. Read more here -- https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/Class.html#getEnumConstants()

Here is the finished product.

private void selectUnit(ActionEvent e) {

    final String value = view.getCategoryBox().getSelectedItem().toString();

    final Class<?> clazz =
        switch (value)
        {

            case "LENGTH"      -> Length.class;
            case "WEIGHT"      -> Weight.class;
            case "TEMPERATURE" -> Temperature.class;
            case "TIME"        -> Time.class;
            case "VOLUME"      -> Volume.class;
            case "AREA"        -> Area.class;
            case "SPEED"       -> Speed.class;
            case "ENERGY"      -> Energy.class;
            default            -> null;

        }
        ;

    if (clazz != null)
    {
        view.getInputBoxUnit().setModel(new DefaultComboBoxModel<>(clazz.getEnumConstants()));
        view.getOutputBoxUnit().setModel(new DefaultComboBoxModel<>(clazz.getEnumConstants()));
    }

}
Sign up to request clarification or add additional context in comments.

Comments

0

You might want to use something like this:

  // this method avoids having to rewrite the calls to two "setModel()"
private void set( Unit units ) {
   view.getInputBoxUnit().setModel( new DefaultComboBoxModel<>( units.values() ));
   view.getOutputBoxUnit().setModel( new DefaultComboBoxModel<>( units.values() ));
}

private void selectUnit( ActionEvent e ) {

     // we replace the *if* with a *switch*.
   switch( view.getCategoryBox().getSelectedItem().toString() ) {
      case "LENGTH": 
         set( Length );         break;
      case "WEIGHT": 
         set( WEIGHT );         break;
      case "TEMPERATURE": 
         set( TEMPERATURE );    break;
      case "TIME": 
         set( TIME );           break;
      case "VOLUME": 
         set( VOLUME );         break;
      case "AREA": 
         set( AREA );           break;
      case "SPEED": 
         set( SPEED );          break;
      case "ENERGY": 
         set( ENERGY );         break;  
   }
}

Another way would be:

  // we create a map
Map<String, Unit> map;

  // add the "Unit" with their respective keys 
void createMap() {
   map = new HashMap<>();
   map.put( "Length", Length );
   map.put( "WEIGHT", WEIGHT );
   map.put( "TEMPERATURE", TEMPERATURE );
   map.put( "TIME", TIME );
   map.put( "VOLUME", VOLUME );
   map.put( "AREA", AREA );
   map.put( "SPEED", SPEED );
   map.put( "ENERGY", ENERGY );
}

  // we call to "set" with the "Unit" obtained from the map
private void selectUnit( ActionEvent e ) {
   set( map.get( view.getCategoryBox().getSelectedItem().toString() );
}

private void set( Unit units ) {
   view.getInputBoxUnit().setModel( new DefaultComboBoxModel<>( units.values() ));
   view.getOutputBoxUnit().setModel( new DefaultComboBoxModel<>( units.values() ));
}

5 Comments

Thanks, this is definitely better. But now, where is the method Unit.values() coming from? To my understanding, Unit is the parent type of the various different enums. And while the enums themselves do have the method values(), the type Unit does not. At least, not originally. So, if you are providing this values() method, can you edit your answer to show its method body?
The definition of Unit is provided by the OP. (as Units, which is not a good name, since it is plural, perhaps it should be changed to: ListOfUnits).
Sorry, I don't understand what you mean. The definition of Unit is nowhere in the OP. Where do you see it? I am asking for the stuff within the curly braces of interface Unit {}.
Although in the code that accompanies the question, it is not defined, in the text reference is made to JComboBox<Units>: JComboBox<Units>, which implies that the OP. has it defined.
Well, I understand you now. I don't think it's safe to make that assumption (and more importantly, that method is likely where most of the complexity is lol, and therefore, should be explicitly defined). But I get what you are saying now.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.