How to reduce Cyclomatic Complexity by refactoring the code?

Question

The question is repeated one but still I am asking because by using the method suggested in the solution I am not able to reduce complexity significantly. The functions complexity is 28 and I have to reduce it below 10.

private void adjustViewport(IEditorPart editorPart, LineRange range,
    TextSelection selection) {
    ITextViewer viewer = EditorAPI.getViewer(editorPart);
    if (viewer == null) +1
        return; +1

    IDocument document = viewer.getDocument();
    LineRange viewportOfViewer = EditorAPI.getViewport(viewer);

    if (viewportOfViewer == null || document == null) +1 +1
        return; +1

    int lines = document.getNumberOfLines();
    int rangeTop = 0;
    int rangeBottom = 0;
    int selectionTop = 0;
    int selectionBottom = 0;

    if (selection != null) { +1
        try {
            selectionTop = document.getLineOfOffset(selection.getOffset());
            selectionBottom = document.getLineOfOffset(selection
                .getOffset() + selection.getLength());
        } catch (BadLocationException e) { +1
            // should never be reached
            LOG.error("Invalid line selection: offset: "
                + selection.getOffset() + ", length: "
                + selection.getLength());

            selection = null;
        }
    }

    if (range != null) { +1
        if (range.getStartLine() == -1) { +1
            range = null;
        } else {
            rangeTop = Math.min(lines - 1, range.getStartLine());
            rangeBottom = Math.min(lines - 1,
                rangeTop + range.getNumberOfLines());
        }
    }

    if (range == null && selection == null) +1 +1
        return; +1

    // top line of the new viewport
    int topPosition;
    int localLines = viewportOfViewer.getNumberOfLines();
    int remoteLines = rangeBottom - rangeTop;
    int sizeDiff = remoteLines - localLines;

    // initializations finished

    if (range == null || selection == null) { +1 +1
        topPosition = (rangeTop + rangeBottom + selectionTop + selectionBottom) / 2;
        viewer.setTopIndex(topPosition);
        return; +1
    }

    /*
     * usually the viewport of the follower and the viewport of the followed
     * user will have the same center (this calculation). Exceptions may be
     * made below.
     */
    int center = (rangeTop + rangeBottom) / 2;
    topPosition = center - localLines / 2;

    if (sizeDiff <= 0) { +1
        // no further examination necessary when the local viewport is the
        // larger one
        viewer.setTopIndex(Math.max(0, Math.min(topPosition, lines)));
        return; +1
    }

    boolean selectionTopInvisible = (selectionTop < rangeTop + sizeDiff / 2);
    boolean selectionBottomInvisible = (selectionBottom > rangeBottom
        - sizeDiff / 2 - 1);

    if (rangeTop == 0 +1
        && !(selectionTop <= rangeBottom && selectionTop > rangeBottom +1 +1
            - sizeDiff)) {
        // scrolled to the top and no selection at the bottom of range
        topPosition = 0;

    } else if (rangeBottom == lines - 1 +1
        && !(selectionBottom >= rangeTop && selectionBottom < rangeTop +1 +1
            + sizeDiff)) {
        // scrolled to the bottom and no selection at the top of range
        topPosition = lines - localLines;

    } else if (selectionTopInvisible && selectionBottom >= rangeTop) { +1 +1
        // making selection at top of range visible
        topPosition = Math.max(rangeTop, selectionTop);

    } else if (selectionBottomInvisible && selectionTop <= rangeBottom) { +1 +1
        // making selection at bottom of range visible
        topPosition = Math.min(rangeBottom, selectionBottom) - localLines
            + 1;
    }

    viewer.setTopIndex(Math.max(0, Math.min(topPosition, lines)));
}

Edit: I have reduced the complexity down to 11. How should I reduce it furthur ?

private int setTopPositionUtil(int sizeDiff, int rangeTop, int rangeBottom, int selectionTop, int selectionBottom) {
boolean selectionTopInvisible = (selectionTop < rangeTop + sizeDiff / 2);
boolean selectionBottomInvisible = (selectionBottom > rangeBottom - sizeDiff / 2 - 1);
if (rangeTop == 0 && !(selectionTop <= rangeBottom && selectionTop > rangeBottom - sizeDiff)) { // +1 +1 +1
    // scrolled to the top and no selection at the bottom of range
    topPosition = 0;
} else if (rangeBottom == lines - 1 && !(selectionBottom >= rangeTop && selectionBottom < rangeTop + sizeDiff)) { // +1 +1 +1
    // scrolled to the bottom and no selection at the top of range
    topPosition = lines - localLines;
} else if (selectionTopInvisible && selectionBottom >= rangeTop) { // +1 +1
    // making selection at top of range visible
    topPosition = Math.max(rangeTop, selectionTop);
} else if (selectionBottomInvisible && selectionTop <= rangeBottom) { // +1 +1
    // making selection at bottom of range visible
    topPosition = Math.min(rangeBottom, selectionBottom) - localLines + 1;
}
return topPosition;
}

private int setTopPosition(int localLines, int rangeTop, int rangeBottom,int selectionTop, int selectionBottom) {

// top line of the new viewport
int topPosition;
int remoteLines = rangeBottom - rangeTop;
int sizeDiff = remoteLines - localLines;

// initializations finished

/*
 * usually the viewport of the follower and the viewport of the followed
 * user will have the same center (this calculation). Exceptions may be
 * made below.
 */
int center = (rangeTop + rangeBottom) / 2;
topPosition = center - localLines / 2;

if (sizeDiff > 0) { // +1
    setTopPositionUtil(sizeDiff, rangeTop, rangeBottom, selectionTop, selec);
}

return Math.max(0, Math.min(topPosition, lines)); // +1
}

private void adjustViewport(IEditorPart editorPart, LineRange range,TextSelection selection) {
ITextViewer viewer = EditorAPI.getViewer(editorPart);
if (viewer != null) { // +1

    IDocument document = viewer.getDocument();
    LineRange viewportOfViewer = EditorAPI.getViewport(viewer);

    if (viewportOfViewer != null && document != null) { // +1 +1

        int lines = document.getNumberOfLines();
        int rangeTop = 0;
        int rangeBottom = 0;
        int selectionTop = 0;
        int selectionBottom = 0;

        if (selection != null) { // +1
            try {
                selectionTop = document.getLineOfOffset(selection.getOffset());
                selectionBottom = document.getLineOfOffset(selection
                    .getOffset() + selection.getLength());
            } catch (BadLocationException e) { // +1
                // should never be reached
                LOG.error("Invalid line selection: offset: " +
                    selection.getOffset() + ", length: " +
                    selection.getLength());

                selection = null;
            }
        }

        if (range != null) { // +1
            if (range.getStartLine() == -1) { // +1
                range = null;
            } else {
                rangeTop = Math.min(lines - 1, range.getStartLine());
                rangeBottom = Math.min(lines - 1,
                    rangeTop + range.getNumberOfLines());
            }
        }

        if (range != null && selection != null) { // +1 +1
            viewer.setTopIndex(setTopPosition(viewportOfViewer.getNumberOfLines(),
                rangeTop, rangeBottom, selectionTop, selectionBottom));
        } else {
            viewer.setTopIndex((rangeTop + rangeBottom + selectionTop + selectionBottom) / 2);
        }
    }
}
}

Note:The code now consists of 3 methods. The complexity of the second and third are below 10, however the complexity of setTopPositionUtil is still 11. Any help ?
Sorry for the indentation.

And what is your question? You already identified which elements in the source code "add to the bill". Now what? Do you expect us to do the refactoring work that you probably get paid for; or that is part of some assignment? — GhostCat, Oct 24 '16 at 12:39
I know how to refract a code but I am unable to reduce it below 10. — Ankit Joshi, Oct 24 '16 at 12:44
Maybe it would have helped if you said right on startup: I started at 28; down to 15; and that is how it looks like currently. — GhostCat, Oct 24 '16 at 13:19
Usually cyclomatic complexity indicaftes that your method is doing too much work. You could reduce the complexity by extracting some of your `if`-blocks to separate methods. — QBrute, Oct 24 '16 at 13:23
GhostCat I have reduced the complexity further. Now can you help ? — Ankit Joshi, Oct 24 '16 at 15:09
@QBrute How should I reduce it further ? I can't remove the if-else statements. — Ankit Joshi, Oct 24 '16 at 15:10

GhostCat · Answer 1 · 2016-10-24T13:23:07.163

Your question is very wide, but in order to get you going, you should do the following:

Create unit tests for your code under test.
You use some coverage tool in order to get to 100% coverage by your tests.

In other words: you write so many testcases that you are fully convinced that you have a test for each and any aspect within your production code.

Then you start refactoring. And you keep running your test suite; to ensure that you don't break anything. And you keep running your complexity metric tool; to ensure you going in the right direction.

And when the question is: how do I refactor, then turn to the classics such as Refactoring by Fowler or Clean Code by Martin.

In general, I think you are also a bit focusing on the wrong topic: you primary goal should be to create readable code that is easy to follow. My advise would be to further slice this one huge method into some smaller ones.

score 0 · Answer 2 · edited May 23 '17 at 12:16

As GhostCat already mentioned, you will need to create a bunch of unit tests to test the refactoring.

Some notes about the refactoring that I've made:

avoid (when possible) methods with more than 20-30 lines of code
use meaningful names for the methods (I assume that the method setTopPosition would be better named calculateTopPosition)
do not pass a parameter to a method unless it is absolutely necessary (notice below that the viewer is not passed anymore to the method calculateTopIndex because it only needs its result.
more small methods means simpler testing

Below you can see one approach to your problem:

private void adjustViewport(IEditorPart editorPart, LineRange range, TextSelection selection) {
    ITextViewer viewer = EditorAPI.getViewer(editorPart);

    if (viewer == null) return; // +1

    IDocument document = viewer.getDocument();
    LineRange viewportOfViewer = EditorAPI.getViewport(viewer);

    if (viewportOfViewer == null || document == null) return; // +1

    viewer.setTopIndex(calculateTopIndex(document, viewportOfViewer, range, selection);

}

private int calculateTopIndex(IDocument document, LineRange viewportOfViewer, LineRange range, TextSelection 
        selection) {
    int selectionTop = 0;
    int selectionBottom = 0;
    if (selection != null) { // +1
        try {
            selectionTop = document.getLineOfOffset(selection.getOffset());
            selectionBottom = document.getLineOfOffset(selection
                    .getOffset() + selection.getLength());
        } catch (BadLocationException e) { // +1
            // should never be reached
            LOG.error("Invalid line selection: offset: " +
                    selection.getOffset() + ", length: " +
                    selection.getLength());

            selection = null;
        }
    }

    if (!isRangeValid(range)) { // +1
        return (selectionTop + selectionBottom) / 2;
    }

    int lines = document.getNumberOfLines();

    int rangeTop = Math.min(lines - 1, range.getStartLine());
    int rangeBottom = Math.min(lines - 1,
            rangeTop + range.getNumberOfLines());

    boolean isSelectionPresent = selection != null;
    if (isSelectionPresent) { // +1
        return setTopPosition(viewportOfViewer.getNumberOfLines(), rangeTop, rangeBottom, selectionTop,
                selectionBottom);
    } else {
        return (rangeTop + rangeBottom) / 2;
    }
}

private boolean isRangeValid(LineRange range) {
    return (range != null && range.getStartLine() != -1);
}

How to reduce Cyclomatic Complexity by refactoring the code?

2 Answers2