When Does a Code Smell Smell Not So Smelly?

Sonar Updates Discussions
,
1

I just got the mail about opening up this discussion, so thought I’d give it a try…

Basically, there are hundreds of Code Smell rules. Some of them are pretty straightforward and all our devs agree these are good rules to follow.

But there are others, particularly those I like to call “too many notes” rules. In the 80’s movie Amadeus, there’s a scene where someone tells Mozart his music has “too many notes,” and the doofus emperor (*) agrees. Mozart objects that his music has the exact number of notes required, no more and no less.

So there are rules (many of them parameterized) like don’t have too many lines in a method, or too much complexity (computed by a mechanistic formula), or nest too deeply or whatever. There are other rules that are not really limit-based, but are general prohibitions, like ternaries. Some of these led to big discussions when I talked about enabling them.

From my experience, when a rigid rule is made, many people (who are pressed for time, like we all are) will find a quick-and-dirty fix to pass whatever automated system was flagging them. My favorite “horror story”: as a lowly intern I was assigned some code auditing tasks. (Not sure if that meant they thought highly of me, or poorly of code audits. :slight_smile: ) I saw one example where there was a rigid 100-line limit to functions. When a function expanded past this limit, the dev just split it into two functions, and passed all the intermediate data that was live at the end of the first function into the second function via a bunch of global variables!!!

To use the “smell” metaphor, that was a bit like having a skunk spray the room to mask a tobacco smell!

Anyway, we decided that some of these rules are more like “hints.” So we made our own tag, starting with cstm (see Reserved or open tag names?) to mark certain rules. We have a formal review process, and the policy is that if a smell rule has this tag, you’re allowed to violate the rule, but only if the reviewer agrees that the alternatives are even smellier. And we add notes to the rule description to give more clarification.

How do other people feel about these rules? Do you have similar policies, or do you just turn off those borderline cases?

(*) Historians say the real emperor wasn’t the doofus portrayed in the movie. In fact, most of the portrayals in the movie were way off, starting with Salieri.

5 Likes
3

In my previous company, I can remember removing vertical whitespace to get past “A switch case should not have too many lines” issues. :joy:

Once I got to SonarSource, I got at least some of those rules changed to “too many lines of code” but that’s a small victory.

I agree that threshold rules are always going to be arbitrary by their very nature. 15 points of Cognitive Complexity is okay, but 16 is terrible? Yeah, I get it. Automatic detection blended with human judgment makes a lot of sense when detection has to be arbitrary.

I like your approach, and it makes me think of speed limit enforcement where I live. There’s (I’m told) a law on the books that prevents a ticket from being written for less than 10 mph (yes, we use miles here) over the speed limit. So if the limit is 45 mph, you can get away with 55 mph and you’re probably not risking a ticket until 60 mph if you’re traveling with traffic. But they can haul you off to jail for 20 mph over the limit, so there’s a balance to be struck :sweat_smile:

 
Ann

1 Like
4

“Too many lines” (not “Too many lines OF CODE”) – LOL, hey let’s incentivize getting rid of comments! :slight_smile:

I was thinking if more people have the same experience, you might want to make some “official” tags. Like judgement-call or needs-review or something like that. We use cstm.review with the understanding that it means the reviewer has to agree. I was thinking of a second category, which would say basically “this is a hint, but it’s your call, no reviewer required” but we’re just doing the one for now.

There are some smell rules which discourage certain practices because you CAN do them badly. For instance, you have two rules about ternary operators (? : in Java). One says don’t do them at all, the other says don’t nest them. Of course the non-compliant example for the latter is a horror story, but I argued that nested ternaries are OK if lines are split and then PROPERLY indented (and no tabs, just spaces), so was able to get the rule put into our cstm.review category.

As for speeding tickets, I’m wondering if we could fine devs for SQ issues. You know, for revenue enhanc… er, I mean, safety enforcement!

1 Like
5

I like the comparison with Mozart. For my personal projects (I don’t write code at Sonar) I tend to just mark them as “won’t fix” if I think the complexity is acceptable because I also think there can be good reasons from time to time not to change it.

2 Likes
7

While I agree with your general sentiment, I disagree with the need for a tag. To me, all code smells should be ignored when the alternative is worse than the current code.

For some of them, the justification might be harder to find, but I’m pretty confident I could write counterexamples for all code smells.

8

The purpose of the tag is to say which rules require the code reviewer (we’re now trying to get code reviews for all commits now) to agree with the violation. Right now we only tag 12 Java rules like that. One, for instance, is the one about ternaries. Because people may do things like “A ? B : C” (only A,B,C are all very long expressions). Those tend to be hard to read because of the long distance between A and C. I argued that if you put B and C on separate lines with C right below B, then it’s OK. So basically: can the reviewer figure out the logic at a quick glance?

My suggestion for an “official” tag (rather than having the site make a custom one) was just for convenience. The default settings wouldn’t use this tag, but it would be available if you want to tag your own subset of “reviewer discretion” rules. Just like you can set tags on any rule.

Now, for Java:S121 (use curlies on control structures). can you find examples where you should have them in some cases, but not others? I’d think you’d either say, “well I don’t have any problem without braces” and just deactivate the rule entirely, or you’d leave it on and no exceptions. But that’s just me… :slight_smile:

1 Like
9

I think the motivation here is right – when we introduced Security Hotspots, we recognized that some issues really require a review before deciding if there’s really an issue or not.

While we hope that most rules indicate a real issue that must be fixed, especially in the world of maintainability and code smells where it can be more subjective, maybe there’s something we can learn from hotspots.

At the same time, it’s important to not let everything be arguable. And if something is very arguable, at least it probably doesn’t belong in the built-in Quality Profiles.

10

Well, as I said, in some cases, it’s harder to find good reasons :slight_smile: . I’m usually in favor of this rule, so I would not deactivate it. There are some cases where you have a series of ifs that follow a very similar pattern, and you may want to represent this structure in your code by showing it like an array. In this kind of situation, I think the curlies are not that important, because the visual shape of the code already makes it impossible to add another statement within the if without noticing it. And if I add them, my automatic code formatting tool will reformat what I’ve written into a non-tabular form, so I’d rather not have them.

     if (x < -1 && y < -1) return 1;
else if (x < -1 && y <  1) return 7; 
else if (x <  3 && y < -1) return 4; 
else if (x <  3 && y <  8) return 5;
else return 42;

I believe that, in theory, everything is arguable (this is why we allow marking any issue as “won’t fix”). And I agree that some things are more arguable than others.

11

OK, I was just being a jerk :slight_smile: but that’s actually an interesting counterexample to S121. I guess the issue here is that your IDE gets in the way here (probably there’s a setting to use one-true-brace or some such). Of course, some IDE’s might FORCE the braces there.

Another one where the rule is USUALLY good, but not ALWAYS, is java:S1121, which says don’t stuff like “if (j = foo()) {…” (when “j = foo()” followed by “if (j) {…” is more readable, and more clear you’re not making a classic = vs. == mistake). This is good 99% of the time, and probably why languages like Python don’t even allow it.

But a situation I sometimes have is parsing user input with a big if-else stack. So I might have things like “} else if (i = inp.index(“foo”) >= 0) {…” and I want to use i within the … (to parse what comes after foo) rather than having to recompute it – my sense of good code was formed when CPU cycles were less numerous.

12

I’m clearly not going to disagree with you, since for C++ we actually have both this rule and a rule that goes in the opposite direction :wink:

But C++ is a language where people care about:

  • not wasting CPU cycles
  • having the smallest scope possible for a variable
  • initializing variables with the right value (and then possibly making this variable a const, to make reasoning about the code easier)
1 Like
13

Now don’t tell me both rules were on at the same time! (SQ has some mutually-contradictory rules like where to put braces, but they’re off by default, and you can pick your style preferences and turn on just those rules.)

I’m totally with you on C++. It’s funny to watch Java gradually add features that C had from practically day 1. Like enums in 1.5 (of course done the Java way, so loses all the efficiencies). And at a meeting, someone started talking about features that were put into Java 9, like “hey use these.” One of them, I forget what it was, but I pulled my dog-eared K&R C and sent a screenshot of the same feature.

Then again I guess Java never actually claimed to be a numerical perf language. And it is cleaner in many respects.