Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

According to code climate (static quality analysis), a class is F, aka the worst mark you could be granted.

I did submit a pull request to rearrange it here

Which has been refused because the new code is supposedly too abstract.

I'm utterly convinced a F has a real meaning and should be fixed, above all when it's one of the main class of a project.

What's your point of view?

share|improve this question

closed as primarily opinion-based by Neil, GlenH7, MichaelT, gnat, Kilian Foth Jan 10 at 13:43

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

    
itsto connect and exchange with amqp, the first link provides all details of the F –  apneadiving Jan 9 at 12:10

3 Answers 3

Why would an arbitrary rating by an automated system have a “real meaning”? This isn't some universal truth passed down from heavens, it's just an estimate on the very subjective matter of code quality.

A Lint report is only as good as the programmer interpreting it. For each issue detected we can decide:

  • Does this constitute a real problem? If so, fix it.
  • Is this a potential problem which is acceptable in this context? If so, leave it. Or possibly add annotations in the code to skip this section.
  • Is this code of debatable quality, but it follows the project's general style? If so, leave it. If possible, edit your analysis tool's configuration to ignore this type of issue.

Changing code solely in order to game some metric is futile, as it usually just hides the real problem. One common example is trying to reduce the complexity of a function by moving some code blocks into helpers. This can be good (when each helper has a clearly defined job) and encourage code re-use. But it can also mean that code that was previously located directly in the context where it was used now sprawls all over the place: essentially, spaghetti code instead of useful abstraction.

Especially complexity metrics should sometimes not be reduced. Humans and computers understand the complexity of a piece of code differently, most evident in switch/case-like constructs which have higher complexity as one might expect, and dispatch tables that hide equal complexity of a switch inside a data structure. Optimize for a human reader of the code.

In the case of your review: You made some good changes, you made some debatable changes. And you added hundreds of lines, and some new files. Are you sure this actually made the code easier to understand? Note for example the absence of documentation to understand what your new abstractions do. Did you make the changes this way because you wanted to make that software more stable, feature rich, and easier to develop? Or did you just want to hide complexity from an analysis tool?

Any fool can write code that a computer can understand.
Good programmers write code that humans can understand.
                – Martin Fowler

share|improve this answer
1  
I dont really care about the metric, I read the code, couldnt understand, and hopefuly found some neutral tool having the same point of view –  apneadiving Jan 9 at 12:33
5  
You couldn't understand the code so you rewrote it? That sounds... dangerous. –  David Richerby Mar 12 at 11:52

I would definitely try again. I don't think they disagreed it needed refactoring, just with this particular attempt. It's not really an improvement if a static analysis tool thinks it's cleaner, but a human doesn't. Static analysis can't take style into account, or which parts of a language and its libraries the maintainers are most comfortable with.

There's more than one way to refactor something. Try again to see if you can satisfy both humans and static analysis. You might want to post it on codereview.SE to get some specific suggestions.

share|improve this answer

Make smaller, more focused commits. Rename a variable here, extract a method there. Small commits that are obvious improvements are more likely to be picked up than "omnibus" commits.

The maintainer thinks you used too big a hammer when you introduced handlers. Look for simpler refactorings that will improve the code: For example, Hutch::Broker#publish could benefit by moving much of what it does into a couple of private methods, e.g. #ensure_connection_ready.

share|improve this answer

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