How to refactor this code?

Question

Please suggest in refactoring this code. Avoid code duplication, mutiple If's

public FormDataDTO getDataForFieldFormCHrzntalField(Field field) {
     FormDataDTO formDataDTO = new FormDataDTO();
     CHrzntalField cHrzntalField = (CHrzntalField) field;
     for (int j = 0; j < cHrzntalField.getFieldCount(); j++) {
      Field sField = cHrzntalField.getField(j);
      if (sField instanceof LabelField) {
       LabelField labelField = sField;
       String fieldName = labelField.getText();
       System.out.println("The Label field name is " + fieldName);
       formDataDTO.setFieldName(fieldName);
      } else if (sField instanceof CTextFieldBorder) {
       CTextFieldBorder cTextFieldBorder = (CTextFieldBorder) sField;
       Field ssField = cTextFieldBorder.getField(0);
       if (ssField instanceof TextField) {
        TextField textField = ssField;
        System.out.println("Inside TextField---- "
          + textField.getText());
        formDataDTO.setFieldType("TextField");
        formDataDTO.setSelectedValue(textField.getText());
       } else if (ssField instanceof DateField) {
        DateField dateField = ssField;
        String dateString = dateField.toString();
        System.out.println("dateString " + dateString);
        formDataDTO.setFieldType("Date");
        formDataDTO.setSelectedValue(dateString);
       }
      } else if (sField instanceof CChoiceField) {
       CChoiceField cChoiceField = (CChoiceField) sField;
       int i = cChoiceField.getSelectedIndex();
       String selectedValue = cChoiceField.getChoice(i);
       System.out.println("Choice " + selectedValue);
       formDataDTO.setFieldType("Combo");
       formDataDTO.setSelectedValue(selectedValue);
      } else if (sField instanceof CheckboxField) {
       CheckboxField checkboxField = (CheckboxField) sField;
       boolean checkStatus = checkboxField.getChecked();
       System.out.println("Check box field " + checkStatus);
       formDataDTO.setFieldType("Checkbox");
       String status = new Boolean(checkStatus).toString();
       formDataDTO.setSelectedValue(status);
      }
     }
     return formDataDTO;
    }

Answer 1

You could start by pulling each case block (the code inside the if / else if blocks) into their own methods. There isn't a lot of repetition that I can see, it's just trying to do too much in one method.

Answer 2

You should use method overloading to avoid instanceof calls. Each if (sField instanceof ...) should be moved to a separate method taking the desired type as parameter.

Answer 3

You could apply a strategy pattern from the looks of it;

• create an interface with methods you call on all Fields, say FieldHandler
• initialise a map from ClassName to FieldHandler containing implementations per field type you need to cover (like LabelFieldHandler, DateFieldHandler, etc.)
• in your function doXXX instead of using instanceOf to execute variantions per field type, look up the corresponding handler in your map and delegate the call to the handler.

pseudo code:

field = getField(j);
handler = handlerMap.get(field.className);
if (null == handler) {
  // error unknown field type
} else {
  handler.setFormData(field, formDataDTO);
}

Answer 4

First step is to create a unit test verifying the behavior of this method. Secondly, "Tell, don't ask" is a principle of good OO design, so it would be good if you could refactor the Field type and its subclasses, to implement a method that allows them to set the necessary information on the FormDataDTO.

Answer 5

Add a new abstract method in Field

public class Field {
    public abstract void updateFormData(FormDataDTO formDataDTO);
}

and then, implements it in each subclass of Field.

Finally, your code becomes:

public FormDataDTO getDataForFieldFormCHrzntalField(Field field) {
    FormDataDTO formDataDTO = new FormDataDTO();
    CHrzntalField cHrzntalField = (CHrzntalField) field;
    for (int j = 0; j < cHrzntalField.getFieldCount(); j++) {
            Field sField = cHrzntalField.getField(j);
            sField.updateFormData(formDataDTO);
    }
    return formDataDTO;
}

Answer 6

You need to dispatch on field type. There are various ways of doing this:

1. Use if statements that explicitly test the class.

2. Make all fields implement an interface, implement that interface appropriately for each field type, and then call the interface.

3. Use a map to look-up the appropriate action for the class.

Option 1 is what you are doing now; 2 is what Stroboskop mentions; 3 is called the strategy pattern by rsp. 1 is a bit of a mess, as you can see. 2 couples the work of the method above with the fields, while 3 doesn't. Which of these (2 or 3) to chose depends on your particular case. An advantage of (2) is that you don't forget to write the code for each new field (because you'll get a compiler error if you do forget). An advantage of (3) is that if you want to do this kind of thing many times, the fields can get cluttered. Also, (2) requires that you have access to the fields code.

It's worth noting that if you were using Scala rather than Java some of the problems with (2) are avoided with traits (and that it also has nicer syntax for (1) with pattern matching).

personally I would prefer (2) if possible - perhaps implementing it with delegation. The only real advantage of (3) over (1) is that the code is neater - and there's a little extra type safety.