Take the 2-minute tour ×

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);
    		} 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());
    			} else if (ssField instanceof DateField) {
    				DateField dateField = ssField;
    				String dateString = dateField.toString();
    				System.out.println("dateString " + dateString);
    		} else if (sField instanceof CChoiceField) {
    			CChoiceField cChoiceField = (CChoiceField) sField;
    			int i = cChoiceField.getSelectedIndex();
    			String selectedValue = cChoiceField.getChoice(i);
    			System.out.println("Choice " + selectedValue);
    		} else if (sField instanceof CheckboxField) {
    			CheckboxField checkboxField = (CheckboxField) sField;
    			boolean checkStatus = checkboxField.getChecked();
    			System.out.println("Check box field " + checkStatus);
    			String status = new Boolean(checkStatus).toString();
    	return formDataDTO;
share|improve this question
The feeling of this question is that it should be in a coding exam. The 'Refactor-my-code' tag is a bit of a giveaway that you seem to be asking someone else to do your work for you. –  Darren Oster Nov 10 '09 at 9:45
Hints/suggestions is all I'm expecting. I just used the existing tag. :) –  HanuAthena Nov 10 '09 at 9:54
I don't see any problem with the question. For a start he's asked for suggestions to refactor the code. It's not like the question says "I need to write an application to xyz, please send me teh codez". Maybe once a solution is decided upon the question could be editted to 'refactoring xxx to yyy' for future reference to people. –  Kirschstein Nov 10 '09 at 9:55
Fair call. I am sufficiently chastised. –  Darren Oster Nov 10 '09 at 9:59

6 Answers 6

up vote 4 down vote accepted

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.

share|improve this answer
welcome to SO :) –  HanuAthena Nov 10 '09 at 10:07

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.

share|improve this answer

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;
            Field sField = cHrzntalField.getField(j);
    return formDataDTO;
share|improve this answer
This would be a solution unless the Field types are not part of the project but of the encapsulating framework. –  rsp Nov 10 '09 at 11:45
Yes, exactly. (on both counts: rsp has a point) –  CPerkins Nov 10 '09 at 11:58
ok, then you can use the same thing, but using a new FieldWrapper interface, that will encapsulate the Field instance, and that could be created using a factory or the strategy pattern. –  Lastnico Nov 10 '09 at 13:17

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);
share|improve this answer

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.

share|improve this answer
That won't work because the method is picked at compile time and would be the one that takes a "Field" parameter. You need to cast. –  Stroboskop Nov 10 '09 at 9:50
Stroboskop is right - however, even better would be to create subclasses of Field and implement the logic there. Override, not overload; ask, don't tell. –  Andrzej Doyle Nov 10 '09 at 12:05

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.

share|improve this answer

Your Answer


