Refactoring example

Added by Smitha Ajay, last edited by Smitha Ajay on Oct 31, 2005  (view change)

Proceeding to solve the text area bug, i went on to look at the TestAreaReactor.java code.

There was (and still is) enormous potential to refactor the same.

A few refactoring techniques that I followed :

Refactoring is about writing reusable code that is readable by human beings.

In short,basic golden rules :
-Design classes so that they represent only a single main functionality
-make methods as small as possible

The refactoring process :

1) Clean up the constructor or any huge methods :

Quoting from Martin Fowler's Refactoring book ,a method or contructor should be written in such a way that it should be no more than a series of steps like in an alogorithm.

eg: This constructor below is definitely readable :

TestAreaReactor.java
public TextAreaReactor(TextAreaDelegate delegate) {
        super(delegate);
        rows = 0;
        columns = 0;
        this.delegate = delegate;
        text = new StringBuffer();
        caretPosition = 0;
        selectionStart = 0;
        selectionEnd = 0;
        leftClicked = false;
        wrap = false;
        fillControlActions();
        executeFontAction();
        addInEvents();
        addActions();
        addOutEvents();
        executeDefaultTextAction();
        addAllProperties();
    }

Than this below code :

TestAreaReactor.java
public TextAreaReactor(TextAreaDelegate delegate) {
        super(delegate);
        rows = 0;
        columns = 0;
        this.delegate = delegate;
        text = new StringBuffer();
        caretPosition = 0;
        selectionStart = 0;
        selectionEnd = 0;
        leftClicked = false;
        wrap = false;
        fillControlActions();

        //This does not cause the program to stop when a break is set
        //and the user changes the font of the text area.
        //font = new Font("SansSerif", Font.PLAIN, 12);
        Action setFont = new SetFontAction(this);
        setFont.getInputDocks().get(Visual.FONT_DOCK).receive(new DataEvent(TextArea.DEFAULT_FONT));
        setFont.execute();

        inEvents.add("MouseReleased", new MouseReleasedEvent());
        inEvents.add("MousePress", new MousePressEvent());
        inEvents.add("MouseDrag", new MouseDragEvent());
        inEvents.add("KeyTyped", new KeyTypedEvent());
        inEvents.add("ControlKey", new ControlKeyEvent());

        actions.add(TextArea.APPEND_STRING_ACTION, new AppendStringAction());
        actions.add(TextArea.INSERT_STRING_ACTION, new InsertStringAction());
        actions.add(TextArea.REPLACE_STRING_ACTION, new ReplaceStringAction());
        actions.add(TextArea.CLEAR_ACTION, new ClearAction());
        
        actions.add(TextArea.SET_TEXT_ACTION, new SetTextAction());
        actions.add(TextArea.GET_TEXT_ACTION, new GetTextAction());
        Property textProperty = new BasicProperty(
                new GetTextAction(),
                new SetTextAction(),
                new BasicDescriptor("text", 
                        "The String text to be displayed in this text area"),
                        String.class);
        
        actions.add(TextArea.SET_COLUMNS_ACTION, new SetColumnsAction());
        actions.add(TextArea.GET_COLUMNS_ACTION, new GetColumnsAction());
        Property columnsProperty = new BasicProperty(
                new GetColumnsAction(),
                new SetColumnsAction(),
                new BasicDescriptor("columns", 
                        "Number of columns in the text area"),
                        Integer.class);

        actions.add(TextArea.SET_ROWS_ACTION, new SetRowsAction());
        actions.add(TextArea.GET_ROWS_ACTION, new GetRowsAction());
        Property rowsProperty = new BasicProperty(
                new GetRowsAction(),
                new SetRowsAction(),
                new BasicDescriptor("rows", "Number of rows in the text area"),
                Integer.class);

        actions.add(TextArea.SET_CARET_ACTION, new SetCaretAction());
        actions.add(TextArea.GET_CARET_ACTION, new GetCaretAction());
        Property caretProperty = new BasicProperty(new GetCaretAction(),
                new SetCaretAction(), TextArea.CARET_POSITION_DOCK,
                new BasicDescriptor("caret position",
                        "Set the caret (cursor) position within the text area"),
                        Integer.class);

        actions.add(TextArea.SET_SELECTION_ACTION, new SetSelectionAction());
        actions.add(TextArea.GET_SELECTION_ACTION, new GetSelectionAction());

        actions.add(TextArea.GET_SELECTED_TEXT_ACTION,
                new GetSelectedTextAction());

        actions.add(TextArea.ADD_HIGHLIGHT_ACTION, new AddHighlightAction());
        actions.add(TextArea.REMOVE_HIGHLIGHT_ACTION,
                new RemoveHighlightAction());

        actions.add(TextArea.SET_HIGHLIGHT_COLOR_ACTION,
                new SetHighlightColorAction());
        actions.add(TextArea.SET_WORD_WRAP_ACTION, new SetWordWrapAction());
        actions.add(TextArea.GET_WORD_WRAP_ACTION, new GetWordWrapAction());
        actions.add(TextArea.GET_NUMBER_OF_LINES_ACTION,
                new GetNumberOfLinesAction());
        actions.add(TextArea.GET_LINE_ACTION, new GetLineAction());

        outEvents.add("TextWasChanged", new DataEvent(String.class));
        outEvents.add("SelectionWasMade", new SelectionWasMade(0, 0));

        Action defaultText = getActions().get(TextArea.SET_TEXT_ACTION);
        defaultText.getInputDocks().get(TextArea.STRING_DOCK).receive(
                new DataEvent("TextArea"));
        defaultText.execute();

        properties.add(columnsProperty);
        properties.add(rowsProperty);
        properties.add(textProperty);
        properties.add(caretProperty);
    }

As can be seen, the different logical sections in the constructor have been combined together into methods which is very simple given an IDE like Eclipse.

Another example :

TestAreaReactor.java
protected void fillControlActions() {
        controlActions = new HashMap();

        //using BuilderActions for these now, since all they need is execute -
        // James E
        controlActions.put(ControlKeyEvent.ENTER, new BuilderAction() {
            public void execute() {
                receive(new KeyTypedEvent(null, '\n'));
            }
        });
        controlActions.put(ControlKeyEvent.TAB, new BuilderAction() {
            public void execute() {
                receive(new KeyTypedEvent(null, '\t'));
            }
        });
        controlActions.put(ControlKeyEvent.BACKSPACE, new BuilderAction() {
            public void execute() {
                if (!enabled) {
                    return;
                }

                if (selectionStart != selectionEnd) {
                    int start;
                    int end;

                    if (selectionStart > selectionEnd) {
                        start = selectionEnd;
                        end = selectionStart;
                    } else {
                        start = selectionStart;
                        end = selectionEnd;
                    }

                    text.delete(start, end);
                    setDelegateText(text.toString());
                    caretPosition = start;
                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                } else if (caretPosition > 0) {
                    text.deleteCharAt(caretPosition - 1);

                    int oldCaret = caretPosition;
                    setDelegateText(text.toString());
                    caretPosition = oldCaret - 1;
                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                }
            }
        });
        controlActions.put(ControlKeyEvent.DELETE, new BuilderAction() {
            public void execute() {
                if (!enabled) {
                    return;
                }

                if (selectionStart != selectionEnd) {
                    int start;
                    int end;

                    if (selectionStart > selectionEnd) {
                        start = selectionEnd;
                        end = selectionStart;
                    } else {
                        start = selectionStart;
                        end = selectionEnd;
                    }

                    text.delete(start, end);
                    setDelegateText(text.toString());
                    caretPosition = start;
                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                } else if (caretPosition != text.length()) {
                    int oldCaret = caretPosition;
                    text.deleteCharAt(caretPosition);
                    setDelegateText(text.toString());
                    caretPosition = oldCaret;
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                    setDelegateCaret();
                }
            }
        });
        controlActions.put(ControlKeyEvent.UP_ARROW, new BuilderAction() {
            public void execute() {
                if (text.substring(0, caretPosition).lastIndexOf("\n") > 0) {
                    int relativePosition = caretPosition
                            - text.substring(0, caretPosition)
                                    .lastIndexOf("\n");
                    String previousLines = text.substring(0, caretPosition);
                    previousLines = previousLines.substring(0, previousLines
                            .lastIndexOf("\n"));

                    if (previousLines.lastIndexOf("\n") < 0) {
                        caretPosition = relativePosition - 1;
                    } else {
                        if (previousLines.substring(
                                previousLines.lastIndexOf("\n")).length() < relativePosition) {
                            //put caret at end of previous line
                            caretPosition = previousLines.length();
                        } else {
                            //put caret at it's relative position in previous
                            // line
                            caretPosition = previousLines.substring(0,
                                    previousLines.lastIndexOf("\n")).length()
                                    + relativePosition;
                        }
                    }

                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                }
            }
        });
        controlActions.put(ControlKeyEvent.DOWN_ARROW, new BuilderAction() {
            public void execute() {
                if (text.substring(caretPosition, text.length()).indexOf("\n") >= 0) {
                    //start of next line
                    int position = 1
                            + caretPosition
                            + text.substring(caretPosition, text.length())
                                    .indexOf("\n");
                    int offset = caretPosition
                            - text.substring(0, caretPosition)
                                    .lastIndexOf("\n");
                    int attemptedCaret = (position + offset) - 1;

                    if (text.substring(position, text.length()).indexOf("\n") == -1) {
                        if (attemptedCaret > text.length()) {
                            caretPosition = text.length();
                        } else {
                            caretPosition = attemptedCaret;
                        }
                    } else {
                        int endOfNextLine = position
                                + text.substring(position, text.length())
                                        .indexOf("\n");

                        if (attemptedCaret > endOfNextLine) {
                            caretPosition = endOfNextLine;
                        } else {
                            caretPosition = attemptedCaret;
                        }
                    }

                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                }
            }
        });
        controlActions.put(ControlKeyEvent.LEFT_ARROW, new BuilderAction() {
            public void execute() {
                if (caretPosition > 0) {
                    caretPosition--;
                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                }
            }
        });
        controlActions.put(ControlKeyEvent.RIGHT_ARROW, new BuilderAction() {
            public void execute() {
                if (caretPosition != text.length()) {
                    caretPosition++;
                    setDelegateCaret();
                    selectionStart = caretPosition;
                    selectionEnd = caretPosition;
                }
            }
        });
    }

is now refactored into :

TestAreaReactor.java
protected void fillControlActions() {
        controlActions = new HashMap();
        enterEvent();
        tabEvent();
        backspaceEvent();
        deleteEvent();
        upArrowEvent();
        downArrowEvent();
        leftArrowEvent();
        rightArrowEvent();
    }

2)If there is a boolean condition, pull that boolean condition into a meaningful method that represents what the condition does :
For example , consider the below if(boolean) code :

TestAreaReactor.java
if (((MouseReleasedEvent) event).getButtonID() == MouseReleasedEvent.BUTTON1) {..}

The above code would make more sense if it were :

TestAreaReactor.java
if (isSameButton(event)) {....}

3)Try to eleminate as many if() {} loops :

Consider the same example again :

TestAreaReactor.java
if (isSameButton(event)) {
     leftClicked = false;
     executeSetCaretAction(event);
     if (selectionStart == selectionEnd) return;
     executeSetSelectionAction();
     emit(new SelectionWasMade(selectionStart, selectionEnd));
}

The {} in the 'if" condition could be eliminated if replaced with the below code :

TestAreaReactor.java
if (!isSameButton(event)) return;
leftClicked = false;
executeSetCaretAction(event);
if (selectionStart == selectionEnd) return;
executeSetSelectionAction();
emit(new SelectionWasMade(selectionStart, selectionEnd));

May seem very trivial ,nevertheless makes a lot of difference.

Another example :

TestAreaReactor.java
if (text.substring(caretPosition, text.length()).indexOf("\n") >= 0) {....}

could be refactored as below :

TestAreaReactor.java
if (!isStartOfNextLine()) return;

4)Also look for similar code segments in 2 or more places with may be a difference in the variable values.
Such code segments could be made into a new method with the parameters.

For example : consider the below code -

TestAreaReactor.java
private void executeSetSelectionAction() {
		AbstractAction setSelection = new SetSelectionAction();
		setSelection.getInputDocks().get(TextArea.START_INDEX_DOCK)
		        .receive(new DataEvent(new Integer(selectionStart)));
		setSelection.getInputDocks().get(TextArea.END_INDEX_DOCK)
		        .receive(new DataEvent(new Integer(selectionEnd)));
		setSelection.execute();
	}

The below lines of code in the method were repeated in 2-3 places throughout the java class..

TestAreaReactor.java
AbstractAction setSelection = new SetSelectionAction();
	setSelection.getInputDocks().get(TextArea.START_INDEX_DOCK)
	        .receive(new DataEvent(new Integer(selectionStart)));
	setSelection.getInputDocks().get(TextArea.END_INDEX_DOCK)
	        .receive(new DataEvent(new Integer(selectionEnd)));
	setSelection.execute();

5)Inline variables if there is only one instance of the variable,to reduce the number of lines of code .
For example :

TestAreaReactor.java
Property rowsProperty = new BasicProperty(
                new GetRowsAction(),
                new SetRowsAction(),
                new BasicDescriptor("rows", "Number of rows in the text area"),
                Integer.class);


properties.add(rowsProperty)

This code could be refactored as below :

TestAreaReactor.java
property rowsproperty = getRowsProperty();
properties.add(rowsProperty);

and finally :

TestAreaReactor.java
properties.add(getRowsProperty());

6)Try to move the inner classes to separate classes especially if the class is as huge and unweildy as the TextAreaReactor.java.

For example, the below inner class in TextAreaReactor.java could be converted into an independent class :

TestAreaReactor.java
private class GetTextAction extends AbstractGetterAction {
        private OutputDock textDock;

        public GetTextAction() {
            super(TextArea.GET_TEXT_ACTION, TextAreaReactor.this.getIdentifier(), getActionLogger());
        }
        
        public OutputDock getDefaultOutputDock() {
            return textDock;
        }

        protected void fire() {
            textDock.setValue(text.toString());
        }

        public Action getClone() {
            return new GetTextAction();
        }

        protected void fillInputDocks() {
        }

        protected void fillOutputDocks() {
            textDock = new BasicOutputDock(String.class,
                    new BasicDescriptor(TextArea.STRING_DOCK,
                            "Contains the text from the TextArea"));
            outputDocks.add(textDock);
        }

        protected void fillErrorDocks() {}
    }

The independent class looks as below :

GetTextAction.java
class GetTextAction extends AbstractGetterAction {
        private OutputDock textDock;
        private TextAreaReactor reactor;

        public GetTextAction(TextAreaReactor reactor){
        	super(TextArea.GET_TEXT_ACTION, reactor.getIdentifier(), reactor.getActionLogger());
        	this.reactor = reactor;
        }
        
        public OutputDock getDefaultOutputDock() {
            return textDock;
        }

        protected void fire() {
            textDock.setValue(reactor.getText().toString());
        }

        public Action getClone() {
            return new GetTextAction(reactor);
        }

        protected void fillInputDocks() {
        }

        protected void fillOutputDocks() {
            textDock = new BasicOutputDock(String.class,
                    new BasicDescriptor(TextArea.STRING_DOCK,
                            "Contains the text from the TextArea"));
            outputDocks.add(textDock);
        }

        protected void fillErrorDocks() {}
    }

(Note unless required, the access modifiers could be package level.)

This could be moved to a saperate package but there are many methods to be made 'public'...hence deffering that for now.

In general, a good convention to follow would be to group all the public methods ,private methods together.

Enter labels to add to this page:
Please wait 
Looking for a label? Just start typing.

Comments (1)

 

  1. Oct 31, 2005

    Nathan Deckard says:

    In the externalized GetTextAction you keep a reference to a TextAreaReactor. But...

    In the externalized GetTextAction you keep a reference to a TextAreaReactor. But what about TextBoxReactor? Or some other widget that has gettable text? This should probably just keep track of a reactor.

    • nathan