2
\$\begingroup\$

I wrote a library for card games. To demonstrate how it can be used, I also wrote an Exploding Kittens card game as an example.

I'm using CodeClimate to determine cyclomatic complexity. This method in the Exploding Kittens game has complexity too high (6):

def play_card(self, card: KittenCard, player: Player = None, target: Player = None):
    if card.selfcast and player is None:
        raise Exception("You must pass a player who owns the card!")
    if card.targetable and target is None:
        raise Exception("You must pass a target!")
    if not self.ask_for_nope():
        card.effect(player, target)
    else:
        print("Card was noped :(")

My question is, how would you reduce it? It seems really readable and maintainable now, but I want to get rid of the Codeclimate issue. Should I be splitting it up to two functions or does it defeat its purpose and makes the code less maintainable?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Your question would benefit from a description of what it is your code actually does. \$\endgroup\$
    – forsvarir
    Sep 10, 2016 at 8:52
  • 1
    \$\begingroup\$ "Example of usage" may be your personal goal for this piece of code, but the question is "What does it do?" The title should be something like "Validating card choice". And the body should contain something explaining the rules of validation and what they accomplish. It would be better for the Github link to go to a specific revision that matches this version of the code. It's easy for people to get to the current version from there, but it's hard to find the historical revision after you change it. \$\endgroup\$
    – mdfst13
    Sep 10, 2016 at 12:02

1 Answer 1

4
\$\begingroup\$

(This is a new version of the answer based on feedback from iScrE4m who asked the question. Feel free to consul the history for the original, wrong answer.)

Readability

First, it's confusing to write code like this:

if condition:
    do_stuff()
if other_condition:
    do_other_stuff()

Because it can be mistaken with the same code using elif where only one branch is executed. And indeed, that's how I read it first. The solution is simple: add a new line between the two conditions. That's a matter of personal preference though, and some developers prefer to write compactly as you did.

What's not captured by cyclomatic complexity?

Nevertheless, it's difficult to reason about this function, simply because you're using ifs and no elsifs. So we have to think about all options: for each if that's not an elif, what happens if both conditions are true? And I don't know if card.selfcast and player is None, card.targetable and target is None and not self.ask_for_nope() can be all true or not. I'm lucky here because you're raising an exception in the first two so I can infer that you consider them exclusive, but that's more reasoning that I would have liked to make.

However, cyclomatic complexity, as defined by Thomas J. McCabe in his 1976 paper, and used by modern tools, does not capture this information. He says in the beginning of section 2 ("A complexity measure"):

The complexity measure approach we will take is to measure and control the number of paths through a program. This approach, however, immediately raises the following nasty problem: "Any program with a backward branch potentially has an infinite number of paths." Although it is possible to define a set of algebraic expressions that give the total number of possible paths through a (structured) program (1), using the total number of paths has been found to be impractical. Because of this the complexity measure developed here is defined in terms of basic paths - that when taken in combination will generate every possible path.

(1) The appendix explains how it can be done.

It's not entirely clear why it's considered impractical, but I guess it makes it harder to compare programs even if does not capture the issue here. (When McCabe wrote his paper in 1976, Fortran did not have ELSE IF: it only appeared in Fortran 77, but it already had a case satement, like the switch statement in C).

Leaving cyclomatic complexity aside, what are all the paths the original code can take? Two options per if, so that's 2*2*2, ie. 8. Now, using elsif, you only have 4 options, all of them with only one statement. Much better, right?

What's captured by cyclomatic complexity?

One thing that cyclomatic complexity does capture is that "complicated" conditions count more than simple ones. So when you write if card.selfcast and player is None, this actually counts as two ifs. This is why you reach 6: 5 ifs + 1. If you want to stay within the limit, you can simply encapsulate the conditions in their own functions, which would lower the complexity to 4.

Solving your immediate issue

I'm not sure encapsulating the conditions is worth it. In this case, consider just raising the limit: 6 is quite low, and I've seen 10 used elsewhere successfully (maybe because this is the number McCabe used in his original paper, even if he says it's arbitrary). This is just at tool, and if it does not help, then stop using it. :)

\$\endgroup\$
3
  • 2
    \$\begingroup\$ According to codeclimate, elif is the same value as if (radon.readthedocs.io/en/latest/intro.html#cyclomatic-complexity), so the complexity won't change, but you're right about using elifs, I don't know why I thought that I needed ifs in there. The real answer for me though is the limit threshold, With limit of 6, I'm often baffled as to why and how and I end up splitting functions into two when I probably shouldn't. 10 sounds pretty good and I'll start using that, so thanks! \$\endgroup\$
    – iScrE4m
    Sep 11, 2016 at 12:46
  • \$\begingroup\$ @iScrE4m I rewrote my answer again after actually reading the paper. Encapsulating the conditions is one way to make the complexity lower, but I still recommend raising the limit here. \$\endgroup\$ Sep 14, 2016 at 5:53
  • \$\begingroup\$ Thanks! I didn't know that it's this old. That gives some context I needed when considering it \$\endgroup\$
    – iScrE4m
    Sep 14, 2016 at 6:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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