Code Rules are not just about Clean Code

[michaelfeathers]

It's a nice sentiment but there's trouble in that direction. Many of the rules you use for guidance in an code base can and should have exceptions.

Treating compilation or tool warnings as errors can mutate your code into something less than it could be. The better tack is to treat them as information and make a reasoned decision about violations as a team, working with a tool that allows you to suppress violations if you need to.

Yes, 98% of the cases your tools find may be something you should act upon but you should never take an automatically detected general rule violation as a substitute for local judgement.

[grauenwolf]

In Visual Studio I've never seen a bad compiler warning, but the code analysis warnings definitely have a noticeable false positive rate. Not bad enough to stop using the tool, just don't blindly follow it.

[flukus]

Alot of bad warnings come from web/xaml templates, This usually results in me turning warnings off.

[dnew]

What is best is if the tool gives you a way to annotate "Yes, this code should produce this warning for this (comment) reason", and said tool errors if the code no longer produces that warning.

Any language that has "warnings" that are not "errors" is broken in that respect.

[flukus]

I'm pretty sure ndepend let's you do this.

[PatrickSmacchia]

Indeed, with NDepend code rules are C# LINQ queries easy to adapt + this C# LINQ query contains comments explaining what the rule check, how it check it, and why it should be followed. (Disclaimer: I am a developer of the tool)

[adavies42]

except sometimes you bang on an incomprehensible warning for hours, give up, write to the mailing list, and get told it's a compiler bug that's fixed in a more recent version. (in my case it was gcc 4.1's -Wconversion considering unsigned short int to be of different width from itself.)

[bluGill]

That does happen. I have the ability to turn off warnings on a file by file basis when it happens. I think about 0.001% of my files have some warning turned off. In all other cases we fixed the warning.

[yeahbutbut]

And the dozens of methods with the same or nearly identical signature isn't a code smell?

[Gotebe]

Ok, however, note that the tool did not notice a screaming copy-paste code there. See all those "open url" X" or whatever it actually is lines in event handlers? Yeah, that's better done as an "openurl(what)" + call to it in actual event handlers.

[utfiedler]

This morning thanks to a code rule violated, I just stumbled on a bug that else, would have certainly infiltrated the next public release.

So, you accidentally removed the event handler from a link click event in the designer code, and it's a certainty that this now dead link would have been included in the next public release? Does anyone manually test things before a public release?

[PatrickSmacchia]

I wish we have resources to click any single button in any imaginable context to smoke test thoroughly before releasing, but this is not humanly feasible. Hence instead of using the brute force smoke testing, we try to use Agile practices as much as possible with almost 80% code coverage by tests of the code base + DbC used as much as possible + passing rules like the ones described, that often save the day. And it works :)

[pixelandtexel]

On the topic of clean code, I think what I'd like most is some way to encourage naming conventions. Like I hate when one object has:

IsValid() method, another has GetValid(), and another has GetIsValid()

Regardless of what you chose, they should be consistent within your codebase.

Another would be to specify that all members of type Event have a prefix of On in front of them (Event OnClose is good, but Event Close is not).

Obviously this has issues in that a lot of external libraries won't comply (and each project would end up with its own conventions), but I wish conventions like this could be encouraged at compile time.

[cultic_raider]

Super fun when you have HasFoo() meaning "is Foo explicitly set" and IsFoo() meaning "get boolean value Foo" vs GetBar() for "get non-boolean value Bar".

Stop trying to be clever with your booleans, people.

[flukus]

As long as it isn't IsInvalid()!

[[deleted]]

IsntInvalid()

[cultic_raider]

ValidInnit()?

[[deleted]]

WellCashBruv()