Hacker News Reader

Comment by hacoo | Hacker News Reader

Ugh, no. I’ve worked in a codebase where CI would reject changes that had too much ‘code complexity’. You’d constantly have to find clever ways to split up your code, when doing so did not make sense, to appease the complexity checker. Oh yeah, and if you ever make a one-liner change, you might end up being forced to do a full refactor because that one line pushed the complexity threshold over the edge. The results: PITA for developers and worse code. What a crock of shit.

View on HN

I was removing unused functionality, work became held up as the code complexity was too high (never mind I'd just reduced it). I don't know what tool they were using, and they didn't share the output. I asked them to document what they found, got a mouthful on this not being their responsibility.

Sounds like a place to leave as soon as possible.

Unless the tool just meassured for amount of change and flagged it for review, which might make sense, as you can also mess up by removing things, you think are unused.

Just run.

Sounds like a group that didn't understand the tool and just put it in place. Metrics and tools make good servants but poor managers.

I'm sorry about your experience, but from what it seems, your problem was in your team. Any measure can be turned into a bad policy, code complexity is a red herring here.

Couldnt yout just raise the threshold by tweaking the config with your PR instead?

I do this all the time with other automated checkers (linters, etc.). I don't see why this should be different. If another human agrees it shouldnt be a problem.

Because if you change that config file, people responsible for it will be added to review, and will beat you with a stick for touching it without consulting change with them?

That sounds like a people problem that will manifest in all sorts of terrible ways and not really an indictment of code complexity metrics.

Cyclomatic complexity (which I think is the most common one used) also doesn't really map to what people understand as complex. A giant switch statement with one return per case isn't nearly as complicated as two large nested if/else blocks. Nesting almost inevitably means more contexts you need to keep track of when reading code, particularly nester conditionals.

Use the tool, don't become its slave.

Did you also have the same feedback from your IDE?

In other words would it have been less painful if you didn't have to suffer the long iteration times required to get feedback from some remote CI job?

The thing is that cyclomatic complexity for example (most popular complexity measure that linters use), it doesn’t make sense. Most of the time high cyclomatic complexity of a method is indicative of high business logic complexity… which is fine. And dogmatically saying that methods shouldn’t have that many lines and branches and that you should just come up with better abstractions doesn’t help anyone, whereas having closely related functionality concentrated in a single place, rather than synthetically exploded into N different files, well this does help.

I always had good experiences with it. I found abstractions that really did make sense, instead of treating it like a chore.

I’ve found most CI checks like this are a crock. Forgot empty parentheses for that function definition with an arity of 0? Bzzzzzzzt! Sorry! Build failed.

It’s just foolish.

The only reason a CI should ever fail is if it catches a defect from making it into production.

> The only reason a CI should ever fail is if it catches a defect from making it into production.

Mechanical checks for things like formatting rules, linting errors, and reasonable tools to verify code complexity, as long as they don't produce false positives, are all important to run as part of CI.

Reply on news.ycombinator.com