Win a copy of Kotlin in Action this week in the Kotlin forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Reduce Code Complexity  RSS feed

 
Steave John
Greenhorn
Posts: 9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi,

Below is the code which has high complexity for this method mentioned below (Complexity is 15) . I have tried reducing it by simplifying the if loops , but seems like it will still be difficult to maintain the code in case of future changes. Any suggestions how to best tackle this problem. Thanks in advance.

 
Campbell Ritchie
Marshal
Posts: 55678
161
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You appear to have arrived in the wrong forum; this one is for discussing CodeRanch itself. Let's try moving you elsewhere.

Yes, that code is pretty convoluted. What tool are you using which gave you the 15 and how does it calculate that? I suggest you start by writing down carefully what your method is supposed to do. No programming words permitted.
Also why are you using Strings? Those might mean something to users but they mean nothing to the machine. Why are you not using a Status class?
 
Andrew Polansky
Ranch Hand
Posts: 310
18
Linux MS IE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell recommendation to write all down is a good one. You can also try to draw a flowchart that represents your function. When you draw it on paper, you will see what repeats and it will give you idea how to simplify it.
 
Winston Gutkowski
Bartender
Posts: 10573
65
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:Also why are you using Strings? Those might mean something to users but they mean nothing to the machine. Why are you not using a Status class?

Or indeed a Status enum?and then transfer your entire logic to a method inside the enum that returns a Status, viz:

A few other observations:

1. Presumably, you've been told that it's best not to have more than one return statement in a method. There is some merit to that as a general rule, but in this case it simply clutters up your code because the only thing you ever do is store the value you're going to return. What about:?

2. You're using int literals with double fields. It'll work, but it's generally regarded as "sloppy" programming.

3. Those literals (7, -7, 17, 50, etc...) plainly mean something to you, but absolutely nothing to us, so why not give them names?
  public static final double BORDERLINE_BAD_HIGH = 17.0;

Better still, write Problem and Performance classes and put them in there. Indeed, you might even be able to use those classes to break up that nasty logic too.

Good programming (after getting things correct of course) is mostly about writing readable code; and the surest way to get your programs consigned to File 13 is to write ones that nobody understands.

HIH

Winston
 
Jason Bullers
Ranch Hand
Posts: 115
11
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
Better still, write Problem and Performance classes and put them in there. Indeed, you might even be able to use those classes to break up that nasty logic too.

Good programming (after getting things correct of course) is mostly about writing readable code; and the surest way to get your programs consigned to File 13 is to write ones that nobody understands.


This. I started looking at it and thinking about using an enum and putting the behaviour there, but then quickly realized that this method is doing way too much. There's four arguments and from what I can tell, they seem to be fairly disjoint. Conceptually, there doesn't seem to be much of a connection between the start date, the count, and the number of problems. There even seem to be (maybe) two different sets of enums in those strings that get returned.

I'd definitely take a step back and figure out what this method is trying to do. Explain it in plain English, looking for the word "and" in your explanation. That's a good indicator that you have more than one concept that should be split up.
 
Carey Brown
Bartender
Posts: 2980
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
When I have some messy logic I may take the time to create a decision table like this


It helps me understand if I've covered all possible combinations.

Rows are processed top-down. All X's on a row are treated as '&&'.
 
Paul Clapham
Sheriff
Posts: 22472
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There's a lot to be criticized about that code beyond just its cyclomatic complexity, starting with the name of the method and going on from there. It looks like it's real code and not a made-up example intended to reach a particular teaching goal, but still, a code review doesn't seem to be the OP's goal.
 
Andrew Polansky
Ranch Hand
Posts: 310
18
Linux MS IE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paul Clapham wrote:... a code review doesn't seem to be the OP's goal.


So let's come up with some solutions.

My initial idea is something like this:



All of those "else"s can be simply eliminated by using return right away, instead of at the end of the function.

One thing that got me curious is use of the withinRange. There are very often compared the same values in two calls, but one values are positive and other are negative. Like here:

Steave, why "problems" variable can take negative values? If it's possible to convert "problems" to be always positive, you could remove all withinRanges that check negative values, therefore reducing some of the code complexity.

EDIT: Carey Brown noticed that in some cases no return may be fired up, I have added 'return "NEW"' at the end to fix this issue. OP code would return "NEW" string in such case as well
 
Liutauras Vilda
Marshal
Posts: 4631
316
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paul is right, naming is poor.

1. calculateString - natural feeling comes, that something is wrong. Usually calculations are performed with numbers. Try to come up with more accurate method name.
2. parameter count. count of what?
3. parameter problems. Wouldn't expect to be a double. 1.5 problems? Wouldn't expect to be a number at all by looking at variable name. At least change to amountOfProblems, countOfProblems, numberOfProblems or similar.
4. Line 2. Compile error, missing quotes.

Ok, it looks that you're trying to come up with method, which suppose to defines performance level of your program at certain times. Have a look at Log4j (<- link). You might find it useful to log return of this method.

And same as others, don't really understand particular problems you're running into at the moment, nor how this method works.
 
Carey Brown
Bartender
Posts: 2980
46
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My concern with your approach, while clean, may not handle all of the necessary cases. I can see that some of the values may not fire any of the IF's.
 
Andrew Polansky
Ranch Hand
Posts: 310
18
Linux MS IE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Liutauras Vilda wrote:2. parameter count. count of what?

This one could be fixed by simply adding documentation above

Carey Brown wrote:My concern with your approach, while clean, may not handle all of the necessary cases. I can see that some of the values may not fire any of the IF's.

I have updated my code. I have added 'return "NEW"' at the end of the code. OP initializes "status" variable with "NEW" on the beginning of the function, so this is kind of default value to return if no IFs are fired up.
 
Liutauras Vilda
Marshal
Posts: 4631
316
BSD
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Adam Scheller wrote:This one could be fixed by simply adding documentation above
I'd prefer to see self explanatory name, rather than name + explanation of meaning for each poor variable name.
 
Andrew Polansky
Ranch Hand
Posts: 310
18
Linux MS IE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That would be indeed better, Liutauras.
 
Sresh Rangi
Ranch Hand
Posts: 54
5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'd start by replacing the if-else structures with early returns, and introduce some boolean variables for the conditions. This makes it see any pattern, and rearrange the code. I've replaced Date with LocalDate (from Java 8 or JodaTime). They remove the need for the util methods and remove ambiguity of whether it contains a time or not.



Then you can see that if you move "WARNING" earlier, then some of the later conditions become redundant, so you can simplify the logic to:



It doesn't look like the default "NEW" value can be returned. I think it would be misleading to return this value by default so I've replaced it with null. You could remove the last condition and return "HIGHLY_PROBLEMATIC" instead but I like the explicit condition.
 
Winston Gutkowski
Bartender
Posts: 10573
65
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sresh Rangi wrote:I'd start by replacing the if-else structures with early returns, and introduce some boolean variables for the conditions.

I agree with you, but the "redundancy" involved in naming those limits suggests very strongly to me that Steave's program is missing two classes: Problem and Performance.

Even if they're just utility classes, they could be used to wrap those range checks into aptly-named methods, but I think I'd be inclined to make them double wrappers. Either way, he can then decide on the visibility of those constants as a separate issue.

And regardless of the intricacies of the checks required for his method, I'm pretty sure that the "status" it returns should NOT be a String.

@Steave: Have a look at this article to find out why.

Winston
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!