0

The code below takes in an array of package names and dependencies in the format {Package1: Dependency, Package2: Dependency} and puts them in a hashmap to be returned. According to my interviewer, the method has a cyclomatic complexity of 12, which was is too high to be acceptable. However, I have ran metrics on the code which reported that the complexity is actually 2.

Can someone tell me why the complexity would be so high and how to simplify it for a lower cyclomatic complexity? I am at a loss here.

public static HashMap<String, String> parseDependencies(String[] args) {
    HashMap<String, String> pairs = new HashMap<String, String>();
    for (int i = 0; i < args.length; i++) {
        if (!args[i].contains(": ") || args[i].startsWith(": ")) {
            logger.log(Level.WARNING, ENTRY + args[i] + FORMATTING);
            continue;
        }
        String[] entry = args[i].split(": ");
        if (entry.length < 2) {
            pairs.put(entry[0], null);
        } else {
            pairs.put(entry[0], entry[1]);
        }
    }
    return pairs;
}
Robert Lu
  • 441
  • 2
  • 7
  • 18

2 Answers2

1

Different tools calculate cyclomatic complexity slightly differently. I don't know how the interview arrived at 12. Or how you arrived at 2. Roughly speaking, cyclomatic complexity counts the number of conditions, loops, and control flow statements. You have one loop, 3 conditions, one continue, else, return, that gives 7, which looks about right.

The implementation doesn't strike me as "complex", but there are a number of issues with it:

  • Return type should be the interface, Map<String, String> instead of HashMap
  • Should use a for-each loop instead of a counting loop
  • The condition on entry.length can be eliminated if you add a second parameter -1 to split. That way, due to the earlier condition on args[i].contains(": ") and the continue that follows, entry.length will be guaranteed to be at least 2.
  • The repetition of : is not great.

I would worry more about these things than cyclomatic complexity in this particular example.

Lastly, a common technique to reduce cyclomatic complexity is to extract code to helper methods. For example, complex boolean conditions, or even an entire loop body.

janos
  • 120,954
  • 29
  • 226
  • 236
0

First of all, it is surprising that an interviewer is relying on Cyclomatic Complexity to measure code written in an interview. I don't think I'd want to work in an "shop" that did that kind of thing routinely.

However, your code could be made simpler from a CC perspective:

public static HashMap<String, String> parseDependencies(String[] args) {
    HashMap<String, String> pairs = new HashMap<String, String>();
    for (int i = 0; i < args.length; i++) {
        String[] entry = args[i].split(": ");
        if (entry.length != 2 || entry[0].isEmpty()) {
           logger.log(Level.WARNING, ENTRY + args[i] + FORMATTING);
        } else if (entry[1].isEmpty()) {
            pairs.put(entry[0], null);
        } else {
            pairs.put(entry[0], entry[1]);
        }
    }
    return pairs;
}

The logic and control flow paths have been simplified and that should reduce the CC measure.

(I also took the opportunity to correct some apparent mistakes.)

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • Do you mean that cyclomatic complexity should be ignored in general or just in interviews? I find it a really useful metric in static analysis and there is almost always something wrong with code that has a high CC value. – Mick Mnemonic Oct 30 '16 at 07:15
  • Well, a bit of both. It can be useful, but it can also be a problem; e.g. when someone treats meeting artificial CC thresholds as a goal. For some coding problems, a solution with high CC can be better than one that has been restructured to have a lower CC. – Stephen C Oct 30 '16 at 07:47
  • Yes, metrics shouldn't be (ab)used blindly. However, I would like to see an example where you think code with a high CC value is better than its lower CC counterpart. Sure, it can be useful to occasionally write a lengthy `switch-case` statement, but when you're using an OO language, even these could/should be solved declarative through polymorphism. See also [unconditional programming](http://michaelfeathers.typepad.com/michael_feathers_blog/2013/11/unconditional-programming.html). – Mick Mnemonic Oct 30 '16 at 08:21
  • I'm thinking of cases where someone refactors code into multiple subroutines in a way that ignores the semantics of the tasks being performed. – Stephen C Oct 30 '16 at 13:33