Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
4  
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.. –  Jimmy Hoffa Nov 25 '13 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. –  asyncwait Nov 25 '13 at 21:07
    
how are ExtractBlah classes defined? are these from some library or homebrew? –  gnat Nov 25 '13 at 21:13
4  
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. –  Jimmy Hoffa Nov 25 '13 at 21:37
1  
Re Update: Am I the only one who is has "Over Engineered" ringing in my head? –  mattnz Dec 3 '13 at 21:32

6 Answers 6

up vote 32 down vote accepted

Software Engineering Answer:

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

share|improve this answer
8  
+1 for respecting the reason for the rule, not the rule itself –  Radu Murzea Nov 25 '13 at 22:55
4  
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. –  GrizzLy Nov 26 '13 at 9:32
    
+1 for saying bean counters. –  Brandon Dec 15 '13 at 20:31

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);
}
share|improve this answer
    
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... ? –  FrustratedWithFormsDesigner Nov 25 '13 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. –  MichaelT Nov 25 '13 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"). –  mattnz Nov 25 '13 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. –  GlenH7 Nov 26 '13 at 0:02
5  
@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. –  GlenH7 Nov 26 '13 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.

share|improve this answer

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;
   extractorMap.add("H",eh);
   /*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...


Original answer:

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.

share|improve this answer
3  
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. –  asyncwait Nov 25 '13 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. –  FrustratedWithFormsDesigner Nov 25 '13 at 21:12
    
Just adding another IF branch increased CC to +1. –  asyncwait Nov 25 '13 at 21:14

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;
    }
}

//your function
public static long parseTimeValue(String sValue){
    try{
        return TimeValueExtractor.extract(sValue);
    } catch (NumberFormatException e) {
        //LOGGER.warn("Number format exception", e);
        return TimeValueExtractor.DEFAULT_VALUE;
    }
}
share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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