Code Climate - Too Complex Error

Question

I'm using code climate on one of my projects, and I'm getting an error for have "too complex" of code. I'm not sure how to make the code it's calling out less complex? Here it is:

Method:

 def apply_json
    {
      total_ticket_count: payment_details.tickets.count,
      subtotal: payment_details.subtotal.to_f,
      discount: payment_details.discount.to_f,
      fees: payment_details.fees_total.to_f,
      total: payment_details.total_in_dollars.to_f,
      coupon: {
        amount: payment_details.coupon.amount,
        type: payment_details.coupon.coupon_type,
        name: payment_details.coupon.name,
        valid: payment_details.coupon.valid_coupon?,
      }
    }
 end

It's just JSON that I tucked away in a model. Everything on my branch is great expect for this? I'm not sure what to do? Any ideas on how I can make this less complex?

Just shut the code climate up? It is still a suggestion tool, not a must. — Aleksei Matiushkin, Jan 29 '17 at 18:12
Yeah, That is what I'm probably going to do. I was just curious if there was a way to make it less complex. — Bitwise, Jan 29 '17 at 18:14
There is, but I won’t do it. Move `coupon` declaration into a separate private [redundant] method and do `...., coupon: build_coupon(payment_details)`. Still, I won’t do it. — Aleksei Matiushkin, Jan 29 '17 at 18:17
This is fake science, just do yourself a favor and turn these warnings off! — akuhn, Jan 29 '17 at 23:47

spickermann · Accepted Answer · 2017-01-29T18:25:28.740

3

I wouldn't care too much if Code Climate thinks something is too complex, but actually is easy to understand. Code Climate should help you to write better, easy to read code. But it doesn't provide hard rules.

If you really want to change something, you might want to move the generation of the coupon sub hash to the Coupon model, because it only depends on values provided by the coupon association:

def apply_json
  {
    total_ticket_count: payment_details.tickets.count,
    subtotal:           payment_details.subtotal.to_f,
    discount:           payment_details.discount.to_f,
    fees:               payment_details.fees_total.to_f,
    total:              payment_details.total_in_dollars.to_f,
    coupon:             payment_details.coupon.as_json
  }
end

# in coupon.rb
def as_json
  {
    amount: amount,
    type:   coupon_type,
    name:   name,
    valid:  valid_coupon?
  }
end

A similar refactoring can be done with payment_details but it is not sure, where this attribute is coming from and if it is an associated model.

edited Jan 29 '17 at 18:25

answered Jan 29 '17 at 18:20

spickermann

100,941
9
101
131

In very localized blocks like this where the same thing is repeated over and over somtimes it's good to make an alias like `p` that represents `payment_details`. Normally variable names should be longer and more descriptive, but you can be highly abbreviated in a local context if the intent is clear. – tadman Jan 29 '17 at 18:53
But that doesn't change the complexity calculated by Code Climate or does it? – spickermann Jan 29 '17 at 19:12
I'm not sure if verbosity factors into this particular situation but I know it gets cranky if you have absurdly long method names. – tadman Jan 29 '17 at 19:15
I am not sure if I agree that using an alias improves readability. But as I mentioned in my answer the fact that `payment_details` is repeated in every line is certainly a code smell and will benefit from further refactoring. But how to refactored that depends on what `payment_details` is and where is it coming from. – spickermann Jan 29 '17 at 19:23

akuhn · Answer 2 · 2017-01-29T23:45:55.627

Please just ignore the complexity warnings.

They are misguided.

These warnings are based on fake science.

Cyclomatic complexity has been proposed in 1976 in an academic journal and has alas been adopted by tool builders because it is easy to implement.

But that original research is flawed.

The original paper proposes a simple algorithm to compute complexity for Fortran code but does not give any evidence that the computed number actually correlates to readability and understandability of code. Nada, niente, zero, zilch.

Here is their abstract

This paper describes a graph-theoretic complexity measure and illustrates how it can be used to manage and control program complexity. The paper first explains how the graph-theory concepts apply and gives an intuitive explanation of the graph concepts in programming terms. The control graphs of several actual Fortran programs are then presented to illustrate the correlation between intuitive complexity and the graph-theoretic complexity. Several properties of the graph-theoretic complexity are then proved which show, for example, that complexity is independent of physical size (adding or subtracting functional statements leaves complexity unchanged) and complexity depends only on the decision structure of a program.

The issue of using non structured control flow is also discussed. A characterization of non-structured control graphs is given and a method of measuring the "structuredness" of a program is developed. The relationship between structure and reducibility is illustrated with several examples.

The last section of this paper deals with a testing methodology used in conjunction with the complexity measure; a testing strategy is defined that dictates that a program can either admit of a certain minimal testing level or the program can be structurally reduced

Source http://www.literateprogramming.com/mccabe.pdf

As you can see anecdotical evidence only is given "to illustrate the correlation between intuitive complexity and the graph-theoretic complexity" and the only proof is that code can be rewritten to have a lower complexity number as defined by this metric. Which is a pretty non-sensical proof for a complexity metric and very common for the quality of research from that time. This paper would not be publishable by today's standards.

The authors of the paper have not done an user research and their algorithm is no grounded in any actual evidence. And no research has been able to prove a link between cyclomatic complexity and code comprehension since. Not to mention that this complexity metric was proposed for Fortran rather than modern high level languages.

The best way to ensure code comprehension is code review. Just simply ask another person to read your code and fix whatever they don't understand.

So just turn these warning off.

score 1 · Answer 3 · answered Jan 29 '17 at 19:13

You're trying to describe a transformation of data from one complex structure to another using code and that creates a lot of "complexity" in the eyes of a review tool like Code Climate.

One thing that might help is to describe the transformation in terms of data:

PAYMENT_DETAILS_PLAN = {
  total_ticket_count: [ :tickets, :count ],
  subtotal: [ :subtotal, :to_f ],
  discount: [ :discount, :to_f ],
  fees: [ :fees_total, :to_f ],
  total: [ :total_in_dollars, :to_f ],
  coupon: {
    amount: [ :coupon, :amount ],
    type: [ :coupon, :coupon_type ],
    name: [ :coupon, :name ],
    valid: [ :coupon, :valid_coupon? ]
  }
}

This might not seem like a huge change, and really it isn't, but it offers some measurable benefits. The first is that you can reflect on it, you can inspect the configuration using code. The other is once you've laid down this format you might be able to write a DSL to manipulate it, to filter or augment it, among other things. In other words: It's flexible.

Interpreting that "plan" isn't that hard:

def distill(obj, plan)
  plan.map do |name, call|
    case (call)
    when Array
      [ name, call.reduce(obj) { |o, m| o.send(m) } ]
    when Hash
      [ name, distill(obj, plan) ]
    end
  end.to_h
end

When you put that into action this is what you get:

def apply_json
  distill(payment_details, PAYMENT_DETAILS_PLAN)
 end

Maybe that approach helps in other situations where you're doing largely the same thing.

score 0 · Answer 4 · answered Jan 29 '17 at 18:19

You may extract the coupon subhash as the method returning that subhash. It will reduce the code complexity (as for codeclimate) but it's not really necessary. However some people believes that method must have 5 strings or less. And they are right in the most use cases. But it's totally up to you to decide.

def apply_json
  {
    total_ticket_count: payment_details.tickets.count,
    subtotal: payment_details.subtotal.to_f,
    discount: payment_details.discount.to_f,
    fees: payment_details.fees_total.to_f,
    total: payment_details.total_in_dollars.to_f,
    coupon: subhash
  }
 end

def subhash
  {
    amount: payment_details.coupon.amount,
    type: payment_details.coupon.coupon_type,
    name: payment_details.coupon.name,
    valid: payment_details.coupon.valid_coupon?,
  }
end

Code Climate - Too Complex Error

Method:

4 Answers4