Shortening method based on an argument name

Question

As you can see below, I have a method which executes statements based on the first letter of a component firing an ItemEvent:

public void itemStateChanged(ItemEvent ie) {
    if(ie.getSource() == rRadioButton) {
        currentScale = "r";
        ((DefaultEditor) rSpinner.getEditor()).getTextField().requestFocus();
    }
    else if(ie.getSource() == gRadioButton) {
        currentScale = "g";
        ((DefaultEditor) gSpinner.getEditor()).getTextField().requestFocus();
    }
    else if(ie.getSource() == bRadioButton) {
        currentScale = "b";
        ((DefaultEditor) bSpinner.getEditor()).getTextField().requestFocus();
    }
    //...
}

Is it possible to easily shorten the method and make it more automatic?

If the argument was an ActionEvent I could use getActionCommand to assign the value for currentScale but the same number of lines of code would be needed first to setActionCommand for every component.

I could also use setName and getName for components but this will result it the same issue as described above.

Also it would be good to be able to automatically point to a specific Spinner based on a specific RadioButton as you can see in the attached source code.

Maybe there is a possibility to use reflection for this but I don't know if this could work.

Answer 1

Without seeing the greater context in which this code appears such as how the components are constructed, and not having kept up with Spring since 1999, I can only address the immediate code. The above could be converted to use two maps: one for the text fields and another for the currentScale strings.

In the code that builds the components, create the two maps and store them in instance fields of the same class.

private final Map<Object, JTextField> textFieldsByRadioButton = new IdentityHashMap<>();
private final Map<Object, String> scalesByRadioButton = new IdentityHashMap<>();

private void buildComponents() {
    ...
    addRadioButtonSpinnerMapping(rRadioButton, rSpinner, "r");
    addRadioButtonSpinnerMapping(gRadioButton, gSpinner, "g");
    addRadioButtonSpinnerMapping(bRadioButton, bSpinner, "b");
}

private void addRadioButtonSpinnerMapping(JRadioButton button, JSpinner spinner, String scale) {
    textFieldsByRadioButton.put(button, ((DefaultEditor) spinner.getEditor()).getTextField());
    scalesByRadioButton.put(button, scale);
}

public void itemStateChanged(ItemEvent ie) {
    Object source = ie.getSource();
    JTextField textField = textFieldsByRadioButton.get(source);
    if (textField != null) {
        currentScale = scalesByRadioButton.get(source);
        textField.requestFocus();
    }
    ...
}

While it requires slightly more code, it only takes one line to add another spinner.

Answer 2

You repeat the line

((DefaultEditor) bSpinner.getEditor()).getTextField().requestFocus();

in every branch of the if/then. So you could easily shorten it by having the if/then look up of the currentScale be one thing and then have that line after it.

Speaking of lookup, I'd probably have a hash of the currentScale values indexed by the type of the argument. In ActionScript, I'd use a Dictionary for this--I'm not sure what the Java equvalent is.

This part ((DefaultEditor) bSpinner.getEditor()) looks like a "code smell" to me. This suggests that your instance is reaching out into things it has no business knowing about. At the very least, it's a violation of the law of Demeter. Consider providing this through dependency injection as a member variable or providing it as an argument to the method.

It is also possible that the method you show above might be better integrated into the default editor, the spinner, the spinner's editor, or the text field and that object might be in a position to set the currentState of whatever Class you're quoting from above.