18

I was doing a little exploring of a legacy system I maintain, with NDepend (great tool check it out), the other day. My findings almost made me spray a mouthful of coffee all over my screen. The top 3 functions in this system ranked by descending cyclomatic complexity are:

  1. SomeAspNetGridControl.CreateChildControls (CC of 171!!!)
  2. SomeFormControl.AddForm (CC of 94)
  3. SomeSearchControl.SplitCriteria (CC of 85)

I mean 171, wow!!! Shouldn't it be below 20 or something? So this made me wonder. What is the most complex function you maintain or have refactored? And how would you go about refactoring such a method?

Note: The CC I measured is over the code, not the IL.

Community
  • 1
  • 1
JohannesH
  • 6,430
  • 5
  • 37
  • 71
  • Does the cyclomatic complexity concept make sense when you write self-modifying code? – isekaijin Sep 01 '09 at 22:41
  • Current high score: 1156, Previous high 456. I wonder what the original coder was thinking when they got the 1000th if/then/else in the method...Let's go for 1001. – Jon Raynor Mar 06 '15 at 21:18
  • While consulting on a project, I have personally encountered a complexity of 1328. I have also been told by a reliable source that they have a complexity in their code base >2500 !!! – nsc_feabhas Jul 25 '18 at 10:59

4 Answers4

20

This is kid stuff compared to some 1970s vintage COBOL I worked on some years ago. We used the original McCabe tool to graphically display the CC for some of the code. The print out was pure black because the lines showing the functional paths were so densely packed and spaghetti-like. I don't have a figure but it had to be way higher than 171.

What to do

Per Code Complete (first edition):

If the score is:

  • 0-5 - the routine is probably fine
  • 6-10 - start to think about ways to simplify the routine
  • 10+ - break part of the routine into a second routine and call it from the first routine

Might be a good idea to write unit tests as you break up the original routine.

Andrew Cowenhoven
  • 2,778
  • 22
  • 27
  • Good answer! Also consider picking up a copy of Refactoring. http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672 See also http://www.refactoring.com/ – TrueWill Sep 02 '09 at 01:51
  • I read both Refactoring and Code Complete and have them right here by my side along with The Pragmatic Programmer. I was just curious as to how the community tackles this problem. – JohannesH Sep 02 '09 at 06:29
  • @Andrew: Hey, no COBOL, thats cheating... ;) – JohannesH Sep 02 '09 at 06:31
  • 1
    I remember a talk buy some guy promoting a coverage tool. He ran it on the JDK, and there were methods which had either a cyclomatic complexity or NPath complexity (I forget which one) with 56 decimal digits. IOW: in order to even remotely test this method, you would a number of unit tests, encroaching slowly on the number of particles in the known universe, estimated to be somewhere between 1070 and 1090. – Jörg W Mittag Sep 02 '09 at 10:41
  • See Code Complete, 2nd Ed., p. 458 – Jared Beck Jun 02 '17 at 22:25
3

This is for C/C++ code currently shipping in a product:

the highest CC value that I could reliably identify (i.e. I don't suspect the tool is erroneously adding complexity values for unrelated instances of main(...) ):

  • an image processing function: 184
  • a database item loader with verification: 159

There is also a test subroutine with CC = 339 but that is not strictly part of the shipping product. Makes me wonder though how one could actually verify the test case(s) implemented in there...

and yes, the function names have been suppressed to protect the guilty :)

How to change it:

There is already an effort in place to remedy this problem. The problems are mostly caused by two root causes:

  1. spaghetti code (no encapsulation, lots of copy-paste)
  2. code provided to the product group by some scientists with no real software construction/engineering/carpentry training.

The main method is identifying cohesive pieces of the spaghetti (pull on a thread:) ) and break up the looooong functions into shorter ones. Often there are mappings or transformations that can be extracted into a function or a helper class/object. Switching to using STL instead of hand-built containers and iterators can cut a lot of code too. Using std::string instead of C-strings helps a lot.

LaszloG
  • 719
  • 3
  • 5
0

I have found another opinion on this from this blog entry which seems to make good sense and work for me when comparing it against various code bases. I know it's a highly opinionated topic, so YMMV.

  • 1-10 - simple, not much risk
  • 11-20 - complex, low risk
  • 21-50 - too complex, medium risk, attention
  • More than 50 - too complex, can't test , high risk
Murray Foxcroft
  • 12,785
  • 7
  • 58
  • 86
0

These are the six most complex functions in PerfectTIN, which will hopefully go into production in a few weeks:

32      32      testptin.cpp(319): testmatrix
36      39      tincanvas.cpp(90): TinCanvas::tick
53      53      mainwindow.cpp(126): MainWindow::tick
56      60      perfecttin.cpp(185): main
58      58      fileio.cpp(457): readPtin
62      62      pointlist.cpp(61): pointlist::checkTinConsistency

Where the two numbers are different, it's because of switch statements.

testmatrix consists of several order-2 and order-1 for-loops in a row and is not hard to understand. The thing that puzzled me, looking at it years after I wrote it in Bezitopo, is why it mods something by 83.

The two tick methods are run 20 times a second and check several conditions. I've had a bit of trouble with the complexity, but the bugs are nothing worse than menu items being grayed out when they shouldn't, or the TIN display looking wonky.

The TIN is stored as a variant winged-edge structure consisting of points, edges, and triangles all pointing to each other. checkTinConsistency has to be as complex as it is because the structure is complex and there are several ways it could be wrong.

The hardest bugs to find in PerfectTIN have been concurrency bugs, not cyclomatic bugs.

The most complex functions in Bezitopo (I started PerfectTIN by copying code from Bezitopo):

49      49      tin.cpp(537): pointlist::tryStartPoint
50      50      ptin.cpp(237): readPtin
51      51      convertgeoid.cpp(596): main
54      54      pointlist.cpp(117): pointlist::checkTinConsistency
73      80      bezier.cpp(1070): triangle::subdivide
92      92      bezitest.cpp(7963): main

main in bezitest is just a long sequence of if-statements: If I should test triangles, then run testtriangle. If I should test measuring units, then run testmeasure. And so on.

The complexity in subdivide is partly because roundoff errors very rarely produce some wrong-looking conditions that the function has to check for.

What is now tryStartPoint used to be part of maketin (which now has a complexity of only 11) with even more complexity. I broke out the inside of the loop into a separate function because I had to call it from the GUI and update the screen in between.

Pierre Abbat
  • 485
  • 4
  • 10