5
\$\begingroup\$

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<Object, Object>();
                context.setAttribute("history", history);

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

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

                    object = getHistoryFromRedis(timestamp, count, channel, historyType);
                } else {
                    /* Check for history in context */
                    if (historyType == 1) {
                        final Map<Long, String> channelHistory = myHistory.tailMap(Long.parseLong(timestamp));
                        if (!channelHistory.isEmpty()) {
                            for (Map.Entry<Long, String> entry : channelHistory.entrySet()) {
                                object.put(String.valueOf(entry.getKey()), entry.getValue());
                            }
                        }
                    }

                    if (historyType == 2) {
                        final Map<Long, String> channelHistory = myHistory.descendingMap();

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

                            for (Map.Entry<Long, String> entry : channelHistory.entrySet()) {
                                if (counter >= i) {
                                    break;
                                }
                                object.put(String.valueOf(entry.getKey()), entry.getValue());
                                counter++;
                            }
                        }
                    }
                }
            }
        } else {
            LOGGER.info("Invalid Keys or keys deactivated");
        }
    } 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 JSONObject getHistoryFromRedis(final String timestamp, final String count, final String channelId, final int type) {

    LOGGER.info("get history from redis");
    Set<Tuple> set = null;
    JSONObject history = new JSONObject();

    try {
        final Jedis jedis = jedisHelper.getJedisObjectFromPool();

        if (type == 1 && timestamp != null && !timestamp.isEmpty() && !"0".equals(timestamp)) {
            set = jedis.zrangeByScoreWithScores(channelId, timestamp, String.valueOf(System.currentTimeMillis()));
        }

        if (type == 2 && !"0".equals(count)) {
            final int limit = Integer.parseInt(count);
            set = jedis.zrevrangeByScoreWithScores(channelId, "+inf", "-inf", 0, limit);
        }

        if (set != null) {
            history = new JSONObject(set);
        }

        jedisHelper.returnJedisObjectToPool(jedis);

    } catch (NumberFormatException ex) {
        LOGGER.error("Exception in returning history from Redis: " + ex);
    }
    return history;
}
\$\endgroup\$
  • \$\begingroup\$ what's up with your indentation? \$\endgroup\$ – Malachi Nov 29 '13 at 15:01
  • \$\begingroup\$ You haven't called returnJedisObjectToPool in the second method from a finally block, so that is a resource leak hazard. \$\endgroup\$ – abuzittin gillifirca Nov 29 '13 at 15:04
  • \$\begingroup\$ Indentation is okay in the IDE that I use. I couldn't fix it in the editor here. \$\endgroup\$ – Gaurav Bhor Nov 29 '13 at 15:04
  • \$\begingroup\$ @abuzittingillifirca Ya, got it. \$\endgroup\$ – Gaurav Bhor Nov 29 '13 at 15:07
4
\$\begingroup\$

Simon's answer got the basic idea right and got my vote, but I think his answer left many problems intact, and may not have given enough details to actually do the method extraction.

Note the following:

  • In the retrieveHistory method jedis was not being used, which became obvious after refactoring.

  • By getJedisObjectFromPool and returnJedisObjectToPool is called through a new method doWithRedis so that you need not remember to add the finally block each time. (This method can be moved to JedisHelper class, which seems to be its natural home.)

  • Single return statements, declaring variable at the top of the method, and Yoda conditionals are C archaisms. The do not do you any good in Java. And just make your code much harder to read.

I could go over anything you might want elaborated, until then here is the code:

public String retrieveHistory(final JSONObject data) {

    final ServletContext context = ContextManager.getContext();

    final String defaultResult = new JSONObject().toString();

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

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

        if (!jedisHelper.checkValidity(key1, key2)) {
            LOGGER.info("Invalid Keys or keys deactivated");
            return defaultResult;
        }

        if (data.has("timestamp")) {
            String timestamp = data.getString("timestamp");
            return retrieveHistoryByTimestamp(context, channel, timestamp).toString();
        }

        if (data.has("count")) {
            String count = data.getString("count");
            return retrieveHistoryByCount(context, channel, count).toString();
        }

        return defaultResult;

    } catch (JSONException ex) {
        LOGGER.error("JSON Exception: " + ex);
        return defaultResult;
    } catch (NumberFormatException ex) {
        LOGGER.error("NumberFormatException: " + ex);
        return defaultResult;
    }
}

private JSONObject retrieveHistoryByTimestamp(final ServletContext context,
        String channel, String timestamp) {

    NavigableMap<Long, String> myHistory = getHistory(context, channel);

    if (myHistory != null) {
        return getHistoryFromContextByTimestamp(timestamp, myHistory);
    }

    return getHistoryFromRedisByTimestamp(timestamp, channel);
}

private JSONObject retrieveHistoryByCount(final ServletContext context,
        String channel, String count) {
    NavigableMap<Long, String> myHistory = getHistory(context, channel);

    if (myHistory != null) {
        return getHistoryFromContextByCount(count, myHistory);
    }

    return getHistoryFromRedisByCount(count, channel);

}

private JSONObject getHistoryFromContextByTimestamp(String timestamp,
        NavigableMap<Long, String> myHistory) {
    JSONObject object = new JSONObject();

    final Map<Long, String> channelHistory = myHistory
            .tailMap(Long.parseLong(timestamp));
    if (!channelHistory.isEmpty()) {
        for (Map.Entry<Long, String> entry : channelHistory
                .entrySet()) {
            object.put(String.valueOf(entry.getKey()),
                    entry.getValue());
        }
    }

    return object;
}

private JSONObject getHistoryFromContextByCount(String count,
        NavigableMap<Long, String> myHistory) {
    JSONObject object = new JSONObject();
    final Map<Long, String> channelHistory = myHistory
            .descendingMap();

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

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

    return object;
}

private JSONObject getHistoryFromRedisByTimestamp(final String timestamp, final String channelId) {

    return doWithRedis(jedisHelper, new JedisCallback<JSONObject>() {
        @Override
        public JSONObject call(Jedis jedis) {
            LOGGER.info("get history from redis");

            if (timestamp != null && !timestamp.isEmpty()
                    && !"0".equals(timestamp)) {
                Set<Tuple> set = jedis.zrangeByScoreWithScores(channelId, timestamp,
                        String.valueOf(System.currentTimeMillis()));
                if (set != null) {
                    return new JSONObject(set);
                }
            }
            return new JSONObject();
        }
    });

}

private JSONObject getHistoryFromRedisByCount(final String count, final String channelId) {

    return doWithRedis(jedisHelper, new JedisCallback<JSONObject>() {
        @Override
        public JSONObject call(Jedis jedis) {
            LOGGER.info("get history from redis");

            if (!"0".equals(count)) {
                try {
                    final int limit = Integer.parseInt(count);
                    Set<Tuple> set = jedis.zrevrangeByScoreWithScores(channelId, "+inf",
                            "-inf", 0, limit);
                    if (set != null) {
                        return new JSONObject(set);
                    }
                } catch (NumberFormatException ex) {
                    LOGGER.error("Exception in returning history from Redis: " + ex);
                }
            }

            return new JSONObject();
        }
    });

}

private NavigableMap<Long, String> getHistory(
        final ServletContext context, String channel) {
    @SuppressWarnings("unchecked")
    ConcurrentHashMap<Object, Object> history = (ConcurrentHashMap<Object, Object>) context
            .getAttribute("history");
    if (null == history) {
        history = new ConcurrentHashMap<Object, Object>();
        context.setAttribute("history", history);
    } 

    @SuppressWarnings("unchecked")
    NavigableMap<Long, String> myHistory = (NavigableMap<Long, String>) history
            .get(channel);

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

    return myHistory;
}


interface JedisCallback<T> {
    T call(Jedis jedis);
}

private static <T> T doWithRedis(JedisHelper jedisHelper, JedisCallback<T> callback) {
    Jedis jedis = null;
    try {
        jedis = jedisHelper.getJedisObjectFromPool();
        return callback.call(jedis );
    } finally {
        if (jedis != null) {
            jedisHelper.returnJedisObjectToPool(jedis);
        }

}
}

EDIT

Note that after the refactoring only multiple returns you have that is not either - when validating parameters - when checking if a method returned null and we cannot continue

is in the outermost method. Which we cannot eliminate as I did eliminating historyType dispatch parameter. Maybe you can if you know at the calling site whether you want to call with count or timestamp.

\$\endgroup\$
6
\$\begingroup\$

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<Object, Object>();
        context.setAttribute("history", history);

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

    } else {
        doMoreStuff();
    }
}


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

    if (null == myHistory) {
        LOGGER.info("History for this channel not found in Context");
        myHistory = new ConcurrentSkipListMap<Long, String>();
        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<Long, String> channelHistory = myHistory.tailMap(Long.parseLong(timestamp));
    if (!channelHistory.isEmpty()) {
        for (Map.Entry<Long, String> entry : channelHistory.entrySet()) {
            object.put(String.valueOf(entry.getKey()), entry.getValue());
        }
    }
}

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

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

        for (Map.Entry<Long, String> entry : channelHistory.entrySet()) {
            if (counter >= i) {
                break;
            }
            object.put(String.valueOf(entry.getKey()), entry.getValue());
            counter++;
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Metrics like the cyclomatic complexity are only useful when taken in the context of your code. For example, your retrieveHistory() method has numerous calls to JSONObject where it could throw a JSONException. Each of these calls adds one to the complexity (or should do). On the other hand, I think your exceptions are not overly complicated, and the code looks fine.

So, in this case, I would look at the code, and note that with 5 calls to JSONObject.getString(...) and at least 8 if conditionals, that's already a complexity of 13... Then the additional 2 catch blocks, brings you to 15.

For the method, as I look at it, it seems just fine (the complexity does, anyway).

I would be more concerned about two other things (bugs)....

You have a concurrency bug (same bug twice), and a poor Logging bug (multiple times).

Concurrency Bug

Your code:

ConcurrentHashMap history = (ConcurrentHashMap) context.getAttribute("history");
if (null == history) {
    history = new ConcurrentHashMap<Object, Object>();
    context.setAttribute("history", history);
    ...
} else { .... }

This implies that your context is a central store for this history Map, and the Map is used in a multithreaded way.

Unfortunately, I have to ask 'Which Map?`.....

If two threads are each running your code, they may both find that history is null, and they may both create a new instance of history = new ConcurrentHashMap<Object, Object>(); and they may both call context.setAttribute("history", history);... one thread will override the value of the other, and the 'history' for the one thread will be 'lost'. You should create a method/system on your context that uses a synchronized way (or Locks) to ensure that only one thread can get or create a history instance at any one time.

Logging

Your issue here is that you are not logging any exceptions, just the string value of the exceptions. All your lines like:

LOGGER.error("JSON Exception: " + ex);

should instead be:

LOGGER.error("JSON Exception: " + ex.getMessage(), ex);

which will actually log the stack trace of the exception, not just the message. Exceptions without traces are almost useless...

\$\endgroup\$
  • \$\begingroup\$ As far as I know, calling JSONObject.getString any number of times doesn't add complexity, it is the catch (SomeKindOfException) that adds complexity. Source (see "McCabe Cyclomatic Complexity") \$\endgroup\$ – Simon Forsberg Nov 29 '13 at 13:26
  • \$\begingroup\$ catch is not what adds cyclomatic complexity, it is throw (or throws) which adds the execution-path-branch \$\endgroup\$ – rolfl Nov 29 '13 at 13:34
  • \$\begingroup\$ @SimonAndréForsberg - that reference is incomplete.... it should include throw and throws (I imagine the for-money version is complete though) \$\endgroup\$ – rolfl Nov 29 '13 at 13:37
  • \$\begingroup\$ Your concern about concurrency is exactly why we use ConcurrentHashMap for storing the messages. This is the retrieval code. And thanks for the ex.getMessage() part. \$\endgroup\$ – Gaurav Bhor Nov 29 '13 at 14:08
  • 2
    \$\begingroup\$ @GauravBhor: the fact that you're using ConcurrentHashMap in this situation does not protect you from the race condition. See the answer to this question: stackoverflow.com/questions/11778550/… \$\endgroup\$ – Shivan Dragon Nov 29 '13 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.