# Avoid too complex method - Cyclomatic Complexity

Not sure how to go about this method to reduce Cyclomatic Complexity. Sonar reports 13 whereas 10 is expected. I am sure nothing harm in leaving this method as it is, however, just challenging me how to go about obeying Sonar's rule. Any thoughts would be greatly appreciated.

`````` public static long parseTimeValue(String sValue) {

if (sValue == null) {
return 0;
}

try {
long millis;
if (sValue.endsWith("S")) {
millis = new ExtractSecond(sValue).invoke();
} else if (sValue.endsWith("ms")) {
millis = new ExtractMillisecond(sValue).invoke();
} else if (sValue.endsWith("s")) {
millis = new ExtractInSecond(sValue).invoke();
} else if (sValue.endsWith("m")) {
millis = new ExtractInMinute(sValue).invoke();
} else if (sValue.endsWith("H") || sValue.endsWith("h")) {
millis = new ExtractHour(sValue).invoke();
} else if (sValue.endsWith("d")) {
millis = new ExtractDay(sValue).invoke();
} else if (sValue.endsWith("w")) {
millis = new ExtractWeek(sValue).invoke();
} else {
millis = Long.parseLong(sValue);
}

return millis;

} catch (NumberFormatException e) {
LOGGER.warn("Number format exception", e);
}

return 0;
}
``````

All ExtractXXX methods are defined as `static` inner classes. For example, like one below -

``````    private static class ExtractHour {
private String sValue;

public ExtractHour(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
return millis;
}
}
``````

## UPDATE 1

I am going to settle down with a mix of suggestions here to satisfy Sonar guy. Definitely room for improvements and simplification.

Guava `Function` is just a unwanted ceremony here. Wanted to update the question about current status. Nothing is final here. Pour your thoughts please..

``````public class DurationParse {

private static final Logger LOGGER = LoggerFactory.getLogger(DurationParse.class);
private static final Map<String, Function<String, Long>> MULTIPLIERS;
private static final Pattern STRING_REGEX = Pattern.compile("^(\\d+)\\s*(\\w+)");

static {

MULTIPLIERS = new HashMap<>(7);

MULTIPLIERS.put("S", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractSecond(input).invoke();
}
});

MULTIPLIERS.put("s", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractInSecond(input).invoke();
}
});

MULTIPLIERS.put("ms", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractMillisecond(input).invoke();
}
});

MULTIPLIERS.put("m", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractInMinute(input).invoke();
}
});

MULTIPLIERS.put("H", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractHour(input).invoke();
}
});

MULTIPLIERS.put("d", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractDay(input).invoke();
}
});

MULTIPLIERS.put("w", new Function<String, Long>() {
@Nullable
@Override
public Long apply(@Nullable String input) {
return new ExtractWeek(input).invoke();
}
});

}

public static long parseTimeValue(String sValue) {

if (isNullOrEmpty(sValue)) {
return 0;
}

Matcher matcher = STRING_REGEX.matcher(sValue.trim());

if (!matcher.matches()) {
LOGGER.warn(String.format("%s is invalid duration, assuming 0ms", sValue));
return 0;
}

if (MULTIPLIERS.get(matcher.group(2)) == null) {
LOGGER.warn(String.format("%s is invalid configuration, assuming 0ms", sValue));
return 0;
}

return MULTIPLIERS.get(matcher.group(2)).apply(matcher.group(1));
}

private static class ExtractSecond {
private String sValue;

public ExtractSecond(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = Long.parseLong(sValue);
return millis;
}
}

private static class ExtractMillisecond {
private String sValue;

public ExtractMillisecond(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue));
return millis;
}
}

private static class ExtractInSecond {
private String sValue;

public ExtractInSecond(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue) * 1000);
return millis;
}
}

private static class ExtractInMinute {
private String sValue;

public ExtractInMinute(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue) * 60 * 1000);
return millis;
}
}

private static class ExtractHour {
private String sValue;

public ExtractHour(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue) * 60 * 60 * 1000);
return millis;
}
}

private static class ExtractDay {
private String sValue;

public ExtractDay(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue) * 24 * 60 * 60 * 1000);
return millis;
}
}

private static class ExtractWeek {
private String sValue;

public ExtractWeek(String sValue) {
this.sValue = sValue;
}

public long invoke() {
long millis;
millis = (long) (Double.parseDouble(sValue) * 7 * 24 * 60 * 60 * 1000);
return millis;
}
}
``````

}

## UPDATE 2

Though I added my update, it is only that much worth the time. I am going to move on since Sonar now does not complains. Don't worry much and I am accepting the mattnz answer as it is the way to go and don't want to set a bad example for those who bumps on to this question. Bottom line -- Don't over engineer for the sake of Sonar (or Half Baked Project Manager) complains about CC. Just do what's worth a penny for the project. Thanks to all.

• Easiest OO answer offhand: `private static Dictionary<string,Func<string,long>> _mappingStringToParser;` I'll leave the rest as an exercise for you (or someone with more spare time right now than I). There's a cleaner API to be found if you have any familiarity with monadic parsers but I won't go there right now.. Commented Nov 25, 2013 at 20:54
• Would love if you could spare sometime on "monadic parsers" and how it can be applied to pretty small function like this one. And, this piece of code is from Java. Commented Nov 25, 2013 at 21:07
• how are `ExtractBlah` classes defined? are these from some library or homebrew?
– gnat
Commented Nov 25, 2013 at 21:13
• The appended code leads me a little further towards a simpler implementation: Your actual variance is the multiplyer. Create a map of these: Pull the alpha characters from the end of your sValue, use those as your key and then pull all until the alphas from the front for the value which you multiple by the mapped multiplyer. Commented Nov 25, 2013 at 21:37
• Re Update: Am I the only one who is has "Over Engineered" ringing in my head? Commented Dec 3, 2013 at 21:32

This is just one of the many cases where simply counting beans that are simple to count will make you do the wrong thing. Its not a complex function, don't change it. Cyclomatic Complexity is merely a guide to complexity, and you are using it poorly if you change this function based on it. Its simple, its readable, its maintainable (for now), if it gets bigger in the future the CC will skyrocket exponentially and it will get the attention it needs when it needs needs it, not before.

Minion working for a Large Multinational Corporation Answer:

Organizations are full of overpaid, unproductive teams of bean counters. Keeping the bean counters happy is easier, and certainly wiser, than doing the right thing. You need to change the routine to get he CC down to 10, but be honest about why you are doing it - to keep the bean counters off your back. As suggested in comments - "monadic parsers" might help

• +1 for respecting the reason for the rule, not the rule itself Commented Nov 25, 2013 at 22:55
• Well said! However, in other cases where you have CC=13 with several nesting levels and complicated (branching) logic it would be preferable to at least try to simplify it. Commented Nov 26, 2013 at 9:32
• Downvoted this. Cyclomatic complexity's been acknowledged (for some time) as a great indicator for code quality. You're not giving it the credit it deserves. Commented Feb 28, 2020 at 10:29

Thanks to @JimmyHoffa, @MichaelT, and @GlenH7 for their help!

## Python

First things first, you should really only accept known prefixes, that is either 'H' or 'h'. If you have to accept both, you should perform some operation to make it consistent to save room in your map.

In python you could create a dictionary.

``````EXTRACTION_MAP = {
'S': ExtractSecond,
'ms': ExtractMillisecond,
'm': ExtractMinute,
'H': ExtractHour,
'd': ExtractDay,
'w': ExtractWeek
}
``````

Then we want the method to use this:

``````def parseTimeValue(sValue)
ending = ''.join([i for i in sValue if not i.isdigit()])
return EXTRACTION_MAP[ending](sValue).invoke()
``````

Should have a better cyclomatic complexity.

## Java

We only need 1 (one) of each multiplier. Lets put them in a map as some other answers have suggest.

``````Map<String, Float> multipliers = new HashMap<String, Float>();
map.put("S", 60 * 60);
map.put("ms", 60 * 60 * 1000);
map.put("m", 60);
map.put("H", 1);
map.put("d", 1.0 / 24);
map.put("w", 1.0 / (24 * 7));
``````

Then we can just use the map to grab the right converter

``````Pattern foo = Pattern.compile(".*(\\d+)\\s*(\\w+)");
Matcher bar = foo.matcher(sValue);
if(bar.matches()) {
return (long) (Double.parseDouble(bar.group(1)) * multipliers.get(bar.group(2);
}
``````
• Yes, mapping the strings to a conversion factor would be a much simpler solution. If they don't need the objects then they should get rid of them, but I can't see the rest of the program, so maybe they objects are used more as objects somewhere else... ? Commented Nov 25, 2013 at 22:22
• @FrustratedWithFormsDesigner they may, but in the scope of that method, its just returning a long and the instantiated object falls out of scope. As an aside, this has the side effect of if this code is called more frequently, the number of frequently used, short lived objects with no state is cut down.
– user40980
Commented Nov 25, 2013 at 22:34
• Neither of these answers can be considered to provide the same solution as the Original program as they rely on assumptions that may not be valid. Java Code: How are you certain the only thing the methods do is apply a multiplier? Python Code: How are you certain the string is not allowed to contain leading (or mid point) characters other than digits (E.g. "123.456s"). Commented Nov 25, 2013 at 23:16
• @mattnz - have another look at the variables names within the examples provided. It's clear that the OP is receiving a time unit as a string and then needs to convert it to another type of time unit. So the examples provided in this answer pertain directly to the OP's domain. Ignoring that aspect, the answer still provides a generic approach that could be used for a different domain. This answer solves the problem that was presented not the problem that may have been presented.
– user53019
Commented Nov 26, 2013 at 0:02
• @mattnz - 1) OP never specifies that in their example and may not care. How do you know the assumptions are invalid? 2) The general method would still work, potentially requiring a more complicated regex. 3) The point of the answer is to provide a conceptual route to resolving the cyclomatic complexity, and not necessarily a specific, compilable answer. 4) while this answer ignores the broader aspect of "does the complexity matter", it indirectly answers the issue by showing an alternative form for the code.
– user53019
Commented Nov 26, 2013 at 0:29

Since you `return millis` at the end of that awful ifelseifelse anyway, first thing that comes to mind is to return the value immediately from within if-blocks. This approach follows one that is listed in catalog of refactoring patterns as Replace Nested Conditional with Guard Clauses.

A method has conditional behavior that does not make clear what the normal path of execution is

Use Guard Clauses for all the special cases

It will help you get rid of else's, flatten the code and make Sonar happy:

``````    if (sValue.endsWith("S")) {
return new ExtractSecond(sValue).invoke();
} // no need in else after return, code flattened

if (sValue.endsWith("ms")) {
return new ExtractMillisecond(sValue).invoke();
}

// and so on...
return Long.parseLong(sValue); // forget millis, these aren't needed anymore
``````

Another thing worth considering is dropping the try-catch block. This will also lower cyclomatic complexity, but main reason why I recommend it is that with this block, there is no way for caller code to distinguish legally parsed 0 from number format exception.

Unless you're 200% certain that returning 0 for parse errors is what caller code needs, you better propagate that exception up and let caller code decide how to deal with it. It is typically more convenient to decide at caller whether to abort execution or retry getting input, or fall back to some default value like 0 or -1 or whatever.

Your code snippet for an example ExtractHour makes me feel that ExtractXXX functionality is designed in a way far from optimal. I bet every of the remaining classes thoughtlessly repeats same `parseDouble` and `substring`, and multiplying stuff like 60 and 1000 over and over and over again.

This is because you missed the essence of what needs to be done depending on `sValue` - namely, it defines how much chars to cut from the end of the string and what would be the multiplier value. If you design your "core" object around these essential features, it would look like as follows:

``````private static class ParseHelper {
// three things you need to know to parse:
final String source;
final int charsToCutAtEnd;
final long multiplier;

ParseHelper(String source, int charsToCutAtEnd, long multiplier) {
this.source = source == null ? "0" : source; // let's handle null here
this.charsToCutAtEnd = charsToCutAtEnd;
this.multiplier = multiplier;
}

long invoke() {
// NOTE consider Long.parseLong instead of Double.parseDouble here
return (long) (Double.parseDouble(cutAtEnd()) * multiplier);
}

private String cutAtEnd() {
if (charsToCutAtEnd == 0) {
return source;
}
// write code that cuts 'charsToCutAtEnd' from the end of the 'source'
throw new UnsupportedOperationException();
}
}
``````

After that, you'd need a code that configures above objects per particular condition if it is met, or somehow "bypasses" otherwise. This could be done about as follows:

``````private ParseHelper setupIfInSecond(ParseHelper original) {
final String sValue = original.source;
return sValue.endsWith("s") && !sValue.endsWith("ms")
? new ParseHelper(sValue, 1, 1000)
:  original; // bypass
}

private ParseHelper setupIfMillisecond(ParseHelper original) {
final String sValue = original.source;
return sValue.endsWith("ms")
? new ParseHelper(sValue, 2, 1)
: original; // bypass
}

// and so on...
``````

Based on above building blocks, code of your method could look as follows:

``````public long parseTimeValue(String sValue) {

return setupIfSecond(
setupIfMillisecond(
setupIfInSecond(
setupIfInMinute(
setupIfHour(
setupIfDay(
setupIfWeek(
new ParseHelper(sValue, 0, 1))))))))
.invoke();
}
``````

You see, there's no complexity left over, no curly braces inside the method at all (nor multiple returns like in my original brute force suggestion on flattening code). You just sequentially check the input and adjust the processing as needed.

If you really want to refactor it, you could do something like this:

``````// All of your Extract... classes will have to implement this interface!
public Interface TimeExtractor
{
public long invoke();
}

private static class ExtractHour implements TimeExtractor
{
private String sValue;

/*Not sure what this was for - might not be necessary now
public ExtractHour(String sValue)
{
this.sValue = sValue;
}*/

public long invoke(String s)
{
this.sValue = s;
long millis;
millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
return millis;
}
}

private static HashMap<String, TimeExtractor> extractorMap= new HashMap<String, TimeExtractor>();

private void someInitMethod()
{
ExtractHour eh = new ExtractorHour;
/*repeat for all extractors */
}

public static long parseTimeValue(String sValue)
{
if (sValue == null)
{
return 0;
}
String key = extractKeyFromSValue(sValue);
long millis;
TimeExtractor extractor = extractorMap.get(key);
if (extractor!=null)
{
try
{
millis= extractor.invoke(sValue);
}
catch (NumberFormatException e)
{
LOGGER.warn("Number format exception", e);
}
}
else
LOGGER.error("NO EXTRACTOR FOUND FOR "+key+", with sValue: "+sValue);

return millis;
}
``````

The idea is you have a map of keys (what your using in "endsWith" all the time) that map to specific objects that do the processing you want.

It's a bit rough here but I hope it's clear enough. I didn't fill in the details of `extractKeyFromSValue()` because I just don't know enough of what these strings are to do it properly. It looks like it's the last 1 or 2 non-numeric characters (a regex could probably extract it easily enough, maybe `.*([a-zA-Z]{1,2})\$` would work), but I'm not 100% sure...

You could change

``````else if (sValue.endsWith("H") || sValue.endsWith("h")) {
``````

to

``````else if (sValue.toUpper().endsWith("H")) {
``````

That might save you a little bit, but honestly, I wouldn't worry about it too much. I agree with you that I don't think there's much harm in leaving the method as it is. Instead of trying to "obey Sonar's rules" try to "stay close to Sonar's guidelines, as much as is reasonably possible".

You can drive yourself mad trying to follow every single rule that these analysis tools will have in them, but you also have to decide if the rules make sense for your project, and for specific cases where the time spent refactoring just might not be worth it.

• Not much of a use, sonar still complains. I am kind of trying it for fun, at least allows to learn one or two. Code is moved to staging though. Commented Nov 25, 2013 at 21:08
• @asyncwait: Ah, I thought you were taking this report from sonar more seriously than that. Yeah, the change I suggested wouldn't make a huge difference - I don't think it would take you from 13 to 10, but in some tools I've seen this sort of thing make a somewhat noticeable difference. Commented Nov 25, 2013 at 21:12
• Just adding another IF branch increased CC to +1. Commented Nov 25, 2013 at 21:14

To be honest, all of the technical responses above seem terribly complicated for the task at hand. As was already written, the code itself is clean and good, so I would opt for the smallest change possible to satisfy the complexity counter. How about the following refactor:

``````public static long parseTimeValue(String sValue) {

if (sValue == null) {
return 0;
}

try {
return getMillis(sValue);
} catch (NumberFormatException e) {
LOGGER.warn("Number format exception", e);
}

return 0;
}

private static long getMillis(String sValue) {
if (sValue.endsWith("S")) {
return new ExtractSecond(sValue).invoke();
} else if (sValue.endsWith("ms")) {
return new ExtractMillisecond(sValue).invoke();
} else if (sValue.endsWith("s")) {
return new ExtractInSecond(sValue).invoke();
} else if (sValue.endsWith("m")) {
return new ExtractInMinute(sValue).invoke();
} else if (sValue.endsWith("H") || sValue.endsWith("h")) {
return new ExtractHour(sValue).invoke();
} else if (sValue.endsWith("d")) {
return new ExtractDay(sValue).invoke();
} else if (sValue.endsWith("w")) {
return new ExtractWeek(sValue).invoke();
} else {
return Long.parseLong(sValue);
}
}
``````

If I'm counting correctly, the extracted function should have a complexity of 9, which is still passes the requirements. And it's basically the same code as before, which is a good thing, since the code was good to begin with.

Plus, readers of Clean Code might enjoy the fact that the top-level method is now simple and short, while the extracted one deals with details.

You might consider using an enum for storing all your available cases and predicates for matching values. As mentioned before your function is readable enough just to leave it unchanged. Those metrics are there to help you not the other way around.

``````//utility class for matching values
private static class ValueMatchingPredicate implements Predicate<String>{
private final String[] suffixes;

public ValueMatchingPredicate(String[] suffixes) {
this.suffixes = suffixes;
}

public boolean apply(String sValue) {
if(sValue == null) return false;

for (String suffix : suffixes) {
if(sValue.endsWith(suffix)) return true;
}

return false;
}

public static Predicate<String> withSuffix(String... suffixes){
return new ValueMatchingPredicate(suffixes);
}
}

//enum containing all possible options
private static enum TimeValueExtractor {
SECOND(
ValueMatchingPredicate.withSuffix("S"),
new Function<String, Long>(){
public Long apply(String sValue) {  return new ExtractSecond(sValue).invoke(); }
}),

MILISECOND(
ValueMatchingPredicate.withSuffix("ms"),
new Function<String, Long>(){
public Long apply(String sValue) { return new ExtractMillisecond(sValue).invoke(); }
}),

IN_SECOND(
ValueMatchingPredicate.withSuffix("s"),
new Function<String, Long>(){
public Long apply(String sValue) { return new ExtractInSecond(sValue).invoke(); }
}),

IN_MINUTE(
ValueMatchingPredicate.withSuffix("m"),
new Function<String, Long>(){
public Long apply(String sValue) {  return new ExtractInMinute(sValue).invoke(); }
}),

HOUR(
ValueMatchingPredicate.withSuffix("H", "h"),
new Function<String, Long>(){
public Long apply(String sValue) {  return new ExtractHour(sValue).invoke(); }
}),

DAY(
ValueMatchingPredicate.withSuffix("d"),
new Function<String, Long>(){
public Long apply(String sValue) {  return new ExtractDay(sValue).invoke(); }
}),

WEEK(
ValueMatchingPredicate.withSuffix("w"),
new Function<String, Long>(){
public Long apply(String sValue) {  return new ExtractWeek(sValue).invoke(); }
});

private final Predicate<String>      valueMatchingRule;
private final Function<String, Long> extractorFunction;

public static Long DEFAULT_VALUE = 0L;

private TimeValueExtractor(Predicate<String> valueMatchingRule, Function<String, Long> extractorFunction) {
this.valueMatchingRule = valueMatchingRule;
this.extractorFunction = extractorFunction;
}

public boolean matchesValueSuffix(String sValue){
return this.valueMatchingRule.apply(sValue);
}

public Long extractTimeValue(String sValue){
return this.extractorFunction.apply(sValue);
}

public static Long extract(String sValue) throws NumberFormatException{
TimeValueExtractor[] extractors = TimeValueExtractor.values();

for (TimeValueExtractor timeValueExtractor : extractors) {
if(timeValueExtractor.matchesValueSuffix(sValue)){
return timeValueExtractor.extractTimeValue(sValue);
}
}

return DEFAULT_VALUE;
}
}

public static long parseTimeValue(String sValue){
try{
return TimeValueExtractor.extract(sValue);
} catch (NumberFormatException e) {
//LOGGER.warn("Number format exception", e);
return TimeValueExtractor.DEFAULT_VALUE;
}
}
``````

Related to your comment of:

Bottom line -- Don't over engineer for the sake of Sonar (or Half Baked Project Manager) complains about CC. Just do what's worth a penny for the project.

Another option to consider is to change your team's coding standards for situations like this. Perhaps you can add in some sort of team-vote to provide a measure of governance and avoid short-cut situations.

But changing the team's standards in response to situations that don't make sense is a sign of a good team with the right attitude about standards. The standards are there to help the team, not get in the way of writing code.