Hi,
I have to refactor the method kingRange under, the goal being code readability :

/*
For information, here are the class fields needed here :
private String setUp;
public final static Collection validPieces =
    Arrays.asList(new String[] { "R", "N", "B", "K", "Q" });
public static final int LEFT = 1;
public static final int RIGHT = 2;

*/

public Range kingRange() {
    Range result;
    
    if ( (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) > 
             (setUp.indexOf("R") + 1) ) {                
        result = new Range((setUp.indexOf("R") + 1) + 1, (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) - 1);
    }
    else {
        int firstEmptyPositionReadingLeftToRight = 0;
        boolean find = false;
        for (int i = 1; i < 8; i++) {
            if (!find && !validPieces.contains(setUp.substring(i-1,i))){
                firstEmptyPositionReadingLeftToRight = i;
                find = true;
            }
        }
        
        int firstEmptyPositionReadingRightToLeft = 0;
        find = false;
        for (int i = 8; i > 1; i--) {
            if (!find && !validPieces.contains(setUp.substring(i-1,i))){
                firstEmptyPositionReadingRightToLeft = i;
                find = true;
            }
        }

        int left = 0;
        int right = 0;
        if ((setUp.indexOf("R") + 1) ==
               firstEmptyPositionReadingLeftToRight - 1) {
            left = firstEmptyPositionReadingLeftToRight;
        } else {
            left = firstEmptyPositionReadingLeftToRight + 1;
        }
        if ((setUp.indexOf("R") + 1) == 
               firstEmptyPositionReadingRightToLeft + 1) {
            right = firstEmptyPositionReadingRightToLeft;
        } else {
            right = firstEmptyPositionReadingRightToLeft - 1;
        }
        result = new Range(left, right);
     
    }
    return result;
}

Here is where I am so far :

public Range kingRange() {        
    if ( (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) > 
         (setUp.indexOf("R") + 1)) {
        return new Range((setUp.indexOf("R") + 1) + 1, (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) - 1);
    }
    return new Range(rangeLeft(), rangeRight());                
}

private int rangeRight() {
    int firstEmptyPositionReadingRightToLeft = 
        firstEmptyPositionReadingRightToLeft();
    
    if ((setUp.indexOf("R") + 1) == 
            firstEmptyPositionReadingRightToLeft + 1) {
        return  firstEmptyPositionReadingRightToLeft;
    } 
    return firstEmptyPositionReadingRightToLeft - 1;
}

private int rangeLeft() {
    int firstEmptyPositionReadingLeftToRight =
        firstEmptyPositionReadingLeftToRight();
    
    if ((setUp.indexOf("R") + 1) ==
            firstEmptyPositionReadingLeftToRight - 1) {
        return firstEmptyPositionReadingLeftToRight;
    }
    return firstEmptyPositionReadingLeftToRight + 1;
}

private int firstEmptyPositionReadingRightToLeft() {
    for (int i = 8; i > 1; i--) {
        if (!validPieces.contains(setUp.substring(i - 1, i))) {
            return i;
        }
    }
    return 0;
}

private int firstEmptyPositionReadingLeftToRight() {
    for (int i = 1; i < 8; i++) {
        if (!validPieces.contains(setUp.substring(i - 1, i))) {
            return i;
        }
    }
    return 0;
}

My question about it is : do you think that some improvement can still be done using Fowler refactorings (see http://www.refactoring.com, in particular removing duplicate code, merging methods, ...) without sacrificing the code readability and understandability ?

I don't want a solution, just your opinion, and if you want some clues on how to improve, I want to find things by myself.

Thanks.

I think you have done enough refactoring, also put some comments in the code so that it becomes more understandable.

Thanks for your answer, I think that too since to go further it would require to do deep changes, to the detriment of its understandability.

Any other views on it ?

Notice that the only difference in those pairs of methods is the direction of advance, +1 or -1. If you see that much duplication between methods, chances are you can make them one method. Consider how it might work if you defined

final static int LEFT = -1;
final static int RIGHT = 1;

and passed the direction to search as a parameter to a single method.

Thank you for your answers, I've taken all you told me into account, and have been able to pursue my refactoring.

Thanks

This article has been dead for over six months. Start a new discussion instead.