Chat logger using Redis

I'm developing a Java EE project which uses Redis through the Jedis library. I'm retrieving the chat history of a particular channel from the servlet context and if absent, from Redis.

The PMD plugin of Netbeans shows me a message that the Cyclomatic Complexity of the code is 15 which is very large. I wish to reduce that and I need to know if there is a better mechanism which can be used.

```
private transient final org.slf4j.Logger LOGGER = LoggerFactory.getLogger(HistoryRetriever.class);
private transient final JedisHelper jedisHelper = new JedisHelper();

public String retrieveHistory(final JSONObject data) {
final ServletContext context = ContextManager.getContext();
JSONObject object = new JSONObject();
final Jedis jedis = jedisHelper.getJedisObjectFromPool();

try {
String key1, key2, channel, timestamp, count;

key1 = data.getString("key1");
key2 = data.getString("key2");
channel = key2 + "_" + data.getString("channel-id");
timestamp = "";
count = "";
int historyType = 0;

LOGGER.info("History requested for " + channel + " belonging to subkey: " + key2);

if (jedisHelper.checkValidity(key1, key2)) {
if (data.has("timestamp")) {
historyType = 1;
timestamp = data.getString("timestamp");
}
if (data.has("count")) {
historyType = 2;
count = data.getString("count");
}

ConcurrentHashMap history = (ConcurrentHashMap) context.getAttribute("history");
if (null == history) {
history = new ConcurrentHashMap();
context.setAttribute("history", history);

object = getHistoryFromRedis(timestamp, count, channel, historyType);
} else {
ConcurrentSkipListMap myHistory = (ConcurrentSkipListMap) history.get(channel);

if (null == myHistory) {
LO

The easiest, and what I also believe is the best as well, way to reduce cyclomatic complexity is to extract methods. Your method suffers from one particular code smell


Long method: a method, function, or procedure that has grown too large.

Below, I have tried to refactor your code by extracting inner sections into their own methods. The way I have done it here might not be optimal, it might not even be functional, but it will give you an idea of what to do with your code. I have not done the fun part of defining and passing the parameters needed to the separate methods (and making them return something useful).

Essentially what you need to do is to make sure that one method does one thing. "Retrieve history" seems to be a big tasks that needs to be broken down into several subtasks. That is what the new methods should be for.

Here's what I did to reduce your cyclomatic complexity:

public String retrieveHistory(final JSONObject data) {

    final ServletContext context = ContextManager.getContext();
    JSONObject object = new JSONObject();
    final Jedis jedis = jedisHelper.getJedisObjectFromPool();

    try {
        String key1, key2, channel;

        key1 = data.getString("key1");
        key2 = data.getString("key2");
        channel = key2 + "_" + data.getString("channel-id");

        LOGGER.info("History requested for " + channel + " belonging to subkey: " + key2);

        verifyAndDoSomething(key1, key2);
    } catch (JSONException ex) {
        LOGGER.error("JSON Exception: " + ex);
    } catch (NumberFormatException ex) {
        LOGGER.error("NumberFormatException: " + ex);
    } finally {
        jedisHelper.returnJedisObjectToPool(jedis);
        LOGGER.info("History is: " + object.toString());
    }
    return object.toString();
}   

private void doSomething(/* parameters needed goes here */) {
    if (!jedisHelper.checkValidity(key1, key2)) {
        LOGGER.info("Invalid Keys or keys deactivated");
        return;
    }
    String timestamp = "";
    String count = "";
    int historyType = 0;

    if (data.has("timestamp")) {
        historyType = 1;
        timestamp = data.getString("timestamp");
    }
    if (data.has("count")) {
        historyType = 2;
        count = data.getString("count");
    }

    ConcurrentHashMap history = (ConcurrentHashMap) context.getAttribute("history");
    if (null == history) {
        history = new ConcurrentHashMap();
        context.setAttribute("history", history);

        object = getHistoryFromRedis(timestamp, count, channel, historyType);

    } else {
        doMoreStuff();
    }
}

private void doMoreStuff(/* parameters needed goes here */) {
    ConcurrentSkipListMap myHistory = (ConcurrentSkipListMap) history.get(channel);

    if (null == myHistory) {
        LOGGER.info("History for this channel not found in Context");
        myHistory = new ConcurrentSkipListMap();
        history.put(channel, myHistory);

        object = getHistoryFromRedis(timestamp, count, channel, historyType);

    } else {
        /* Check for history in context */
        if (historyType == 1) {
            performHistoryType1();
        }
        if (historyType == 2) {
            performHistoryType2();
        }
    }
}

private void performHistoryType1(/* needed parameters goes here */) {
    final Map channelHistory = myHistory.tailMap(Long.parseLong(timestamp));
    if (!channelHistory.isEmpty()) {
        for (Map.Entry entry : channelHistory.entrySet()) {
            object.put(String.valueOf(entry.getKey()), entry.getValue());
        }
    }
}

private void performHistoryType2(/* same as before, needed parameters goes here */) {
    final Map channelHistory = myHistory.descendingMap();

    if (!channelHistory.isEmpty()) {
        int counter = 0;
        final int i = Integer.parseInt(count);

        for (Map.Entry entry : channelHistory.entrySet()) {
            if (counter >= i) {
                break;
            }
            object.put(String.valueOf(entry.getKey()), entry.getValue());
            counter++;
        }
    }
}

StackExchange Code Review Q#36349, answer score: 6