Today I had a very large If-Else statement, and I needed some way to make it more transparent. I wanted to use a jump table or hash-map to solve this. I would create some key determined by a few common factors, and then assign a method to each key in the hash-map. Unfortunately... to the best of my understanding, Java doesn't allow you to place method calls in has-maps. I would have had to use the Command pattern, make some 'Runnable' classes for each of the methods I wanted, and then stick these runnable objects into the hash-map in place of the methods.
A whole new Command Pattern seemed overkill to simplify my one method with the if-else chain, so instead I did Java's equivalent to a jump table. A switch statement in Java actually compiles directly down to a jump table. I find this a bit annoying because if Java is just going to make a jump table anyway, they should let me make one too. Either way, I used the same basic principle to create a code, and then use the code to select the correct method.
The if statement was being used to decide some word selection criterion. Say you double click and hold on a word, if you then begin moving your cursor over another word, many text areas will jump into a word selection mode and select the entire word at once rather than just following your cursor. So here was the if statement:
if (mouseIndex > boxInfo.findWordsRightEdge(boxInfo.cursorIndex))
{
if (mouseIndex > boxInfo.selectionIndex)
{
boxInfo.selectionIndex = boxInfo.cursorIndex;
boxInfo.cursorIndex = boxInfo.findWordsRightEdge(mouseIndex);
}
else if (boxInfo.cursorIndex < boxInfo.selectionIndex)
boxInfo.cursorIndex = boxInfo.findWordsLeftEdge(mouseIndex);
else
boxInfo.cursorIndex = boxInfo.findWordsRightEdge(mouseIndex);
}
else if (mouseIndex < boxInfo.findWordsLeftEdge(boxInfo.cursorIndex) && mouseIndex > boxInfo.selectionIndex)
{
boxInfo.cursorIndex = boxInfo.findWordsRightEdge(mouseIndex);
}
else if (mouseIndex < boxInfo.selectionIndex && mouseIndex < boxInfo.findWordsLeftEdge(boxInfo.cursorIndex))
{
if (boxInfo.cursorIndex > boxInfo.selectionIndex)
boxInfo.selectionIndex = boxInfo.cursorIndex;
boxInfo.cursorIndex = boxInfo.findWordsLeftEdge(mouseIndex);
}
Unfortunately blogspot makes it rather difficult to display code, and the if statement is hardly readable even properly formated, so good luck reading that...
So to refactor this I created a method object, and used a few criterion to determine an index, and then plugged this into a switch statement. I then made a descriptive method for each action. Here is the refactored code:
public void makeProperWordSelection()
{
switch (calculateSelectionDispatchIndex())
{
case 0:
extendSelectionToRight();
break;
case 1:
extendSelectionToRight();
break;
case 2:
reduceSelectionFromTheLeft();
break;
case 3:
swapSelectionDirectionAndExtendToRight();
break;
case 4:
swapSelectionDirectionAndExtendToLeft();
break;
case 5:
reduceSelectionFromTheRight();
break;
case 6:
extendSelectionToLeft();
break;
}
}
private int calculateSelectionDispatchIndex()
{
int dispatchIndex = 0;
if (mouseIndex > boxInfo.getSelectionIndex())
dispatchIndex += 1;
if (boxInfo.getCursorIndex() < boxInfo.getSelectionIndex())
dispatchIndex += 2;
if (mouseIndex < boxInfo.findWordsLeftEdge(boxInfo.getCursorIndex()))
dispatchIndex += 4;
return dispatchIndex;
}
private void swapSelectionDirectionAndExtendToLeft()
{
boxInfo.setSelectionIndex(boxInfo.getCursorIndex());
boxInfo.setCursorIndex(boxInfo.findWordsLeftEdge(mouseIndex));
}
private void reduceSelectionFromTheRight()
{
boxInfo.setCursorIndex(boxInfo.findWordsRightEdge(mouseIndex));
}
private void extendSelectionToLeft()
{
boxInfo.setCursorIndex(boxInfo.findWordsLeftEdge(mouseIndex));
}
private void reduceSelectionFromTheLeft()
{
boxInfo.setCursorIndex(boxInfo.findWordsLeftEdge(mouseIndex));
}
private void swapSelectionDirectionAndExtendToRight()
{
boxInfo.setSelectionIndex(boxInfo.getCursorIndex());
boxInfo.setCursorIndex(boxInfo.findWordsRightEdge(mouseIndex));
}
private void extendSelectionToRight()
{
boxInfo.setCursorIndex(boxInfo.findWordsRightEdge(mouseIndex));
}
Its debate-able whether or not this is actually better, but I think it at least makes it more readable if you are looking for a specific outcome. It just so happens that it lined up much better with the tests.
I'd like to hear if you think this helps or not, and if not, how you would refactor.
Reading the code really is too hard. Have you considered an online syntax highlighter (like http://tohtml.com) in order to format it properly?
ReplyDeleteI see two problems with the switch statement:
ReplyDelete1. There are magic numbers. The calculateSelectionDispatchIndex() function returns some bitfield, that is not visibile when reading the distinct cases
2. The function is intransparent to the return value by collecting up the desired return value.
What I would start here is refactoring out of the if clauses methods that make up the meaning of the statement. I.e.
if (mouseIndex > boxInfo.findWordsRightEdge(boxInfo.cursorIndex))
is nothing more than
if (mouseIndexIsToTheRightOfCursorIndex())
After finishing these off, I would start to extract methods for the bodies of the if statements. After that I would try to look for the structure that is evolving or leave it as is.
My case here is to make the code more speaking to yourself. When getting back at it in 6 months, you will not directly know what you intended today. So, make the code tell this to your future self.
I will try using a syntax highlighter next time I post code. Thanks Aviv.
ReplyDeleteI completely agree Markus. When I wrote the switch statement, I was moving on the assumption that all that really mattered was understanding the result of the Word Selector rather than how it gets to the result. Thus, as you said, I used these magic numbers that were arbitrarily decided, which hold no clue to how I got them. This hides away all the decision logic, which isn't the direciton I want to go.
Micah and I went back and worked on this and made some pretty drastic changes that you can see on my next post.