My code would suck less if...
My code would suck less if the C# compiler raised an error anytime it detected the following:
- A button with no click handler.
- A click handler with no code.
- A parameter that is not used.
- Code that throws A '
NotImplementedException'
- A
//TODO: token.
- An
#if DEBUG
- A phrase that contains more than one dot, e.g.
Control.Parent.Parent.Parent.Controls[3].Controls["Accept"]
Q: Why do I want the compiler to be so mean?
A: It takes a tough man to make a tender chicken.
(attributed to Purdue, quoted in 'Worse Is Better')
No, the common thread here is these are pitfalls common to a top-down, (or an outside-in) design approach.
As you move top-down, (or from front to back), you throw off "branches" -- kind of last strands -- that you need to back-track to complete later. I want the compiler to help me find those branches. (Tests are a poor substitute)
In his 2005 classic 'Does Visual Studio Rot the Mind?' Charles Petzold showed how intellisense-and-friends encourage bottom-up programming.
These are some little features that could help out the die hard top-down coders.
Details...
"A button with no click handler"
Why does a button exist? Is it just to look good?
No! Buttons are for clicking!
A button with no click handler is a fail.
And a click handler with no code is a fail as well.
What does the user expect when you click a button?
Unless this is that Early Internet Phenomenom known as The Really Big Button That Doesn't Do Anything, a user expects *something* from a button.
Why would you check that in? What good can possible come from letting do-nothing buttons end up in the hands of unsuspecting users?
The world of static code analysis has gone very far – but does it stop and help us avoid this sort of psychological torture?
(This same concept may be true for many other objects that raise events. The object's author might be able to indicate "There's really no point using this thing unless you handle the following event(s).")
A parameter that is not used.
Unused variables raise a warning, but unused parameters don't.
This is a little bit good, but it's a little bit bad as well.
Here's an example where the unused parameter is clearly a bug:
private void CopyFile(string sourcefileName,
string targetFileName,
bool overwrite)
{
System.IO.File.Copy(sourcefileName, targetFileName);
}
Oops!
The programmer forgot all about the 'overwrite' parameter!
I consider that a pretty clear error.
But consider our click handler for a moment:
private void Button1_Click(object sender, EventArgs e)
{
MessageBox.Show("Hello");
}
Neither parameter is used. But if we were to call this an error, a hell of a lot of code that's shipping today would fail.
Some design changes could alleviate the dilemma – I'll leave the consideration of possible solutions as an exercise for the interested reader. The goal is to avoid busy work, not create it.
Code that throws A 'NotImplementedException', and //TODO: token, #if DEBUG conditions and everyone from System.Diagnostics.Debug
Not Implemented Exceptions are excellent stuff.
But your goal is always to eradicate them, right? They're a kind of hardcoded place holder. A warning ought to suffice. (You treat warnings as errors, most of the time, right?)
I put //TODO: tokens and #if DEBUG conditions in the same category. They're very handy scaffolding or place holders as you crank out your code -- but they're not supposed to remain indefinitely. A warning would be nice. Maybe Debug.WriteLine and its friends should fall into the same category.
A phrase that contains more than one dot, e.g. Control.Parent.Parent.Parent.Controls[3].Controls["Accept"]
Why would a compiler let you get away with this kind of monstrosity?
It would be quicker to just replace that whole line with "throw new NullReferenceException()". The effect would be the same.
My point here is that this kind of implementation of the house of cards anti-pattern is easy for static analysis tools to pick up.
I know that there are existing static analysis and code grepping tools that can be used for some of these cases, but they tend to run 'after the fact', outside the tight code-compile loop. Worse: they tend to produce a lot of noise and take a lot of care and feeding.
StyleCop is the worst 'busy work' generator I've seen. (If only they'd finished the job and had it auto-correct many of the problems it finds... instead, like '100% code coverage' it's a kind of training tool for Coder's OCD.)
Alrighty -- that's all I've got for now, though if I keep my eyes open, I bet I'll spot some more.
'Steven Nagy' on Thu, 19 Mar 2009 10:31:01 GMT, sez: Dear Leon,
Long time reader, first time poster.
>> My code would suck less
Having seen your code, I have to attest that it's impossible to imagine the inverse.
I'm bored. Can you write another blog post now? That last one came out wrong.
Waiting in anticipation,
Steven Nagy
'CyberBlox' on Thu, 19 Mar 2009 10:39:36 GMT, sez: My code would suck less...
If it wasn't for those pesky end-users!
'Goran' on Thu, 19 Mar 2009 10:52:17 GMT, sez: A //TODO: token.
Are shown in VS if you click TaskList. Couldn't live without this one :)
'Steven Nagy' on Thu, 19 Mar 2009 10:57:12 GMT, sez: Oh wow I have an impersonator! I bet its OJ... he's always wanted to be like me.
Anyways... My code would suck less if someone else wrote it. eom
'Steven Nagy' on Thu, 19 Mar 2009 11:02:59 GMT, sez: I think I might have Multiple Personality Disorder... And I write my own code, thank you!
'Lucas' on Thu, 19 Mar 2009 12:20:10 GMT, sez: I agree 100%. The good news is that Resharper will catch many of these, and you can configure it to show them as errors, not just warnings or hints.
'Vegar' on Thu, 19 Mar 2009 12:51:16 GMT, sez: For the Control.Parent.Parent-scenario, Delphi Prism has a interesting solution. By substituting dots with colon: http://prismwiki.codegear.com/en/Colon_Operator
'Ken Hughes' on Thu, 19 Mar 2009 13:17:03 GMT, sez: My code would suck less if ... someone else wrote it ;-)
'tarn' on Thu, 19 Mar 2009 14:19:02 GMT, sez: Warnings as error is a start, but I think to much faith is put into compilers anyway.
Way to much time is spent re-compiling entire applications just to make sure the syntax is good, despite that damn blue squiggly underline?!
I'd like to see the background processes running the unit tests in your application, that way validating your codes purpose and the syntax.
'Steven Nagy' on Thu, 19 Mar 2009 19:58:27 GMT, sez: Ken, you stole my line.
To the 3rd Steven Nagy, I know who you are.
In fact you are someone who's already posted a comment on this 'article' (I use the term loosely). Also, you got my URL wrong.
Leon, I think you are attracting the wrong kinds of readers. These ones just seem to be parrots!
'lb' on Thu, 19 Mar 2009 21:18:59 GMT, sez: @Goran:
>TODO token, shown in VS if you
>click TaskList
Yep, since vs2005 it's had its own window, but i'm pretty sure in vs 2002 and 2003 it was integrated straight into the error/warning list.
And nowadays it only seems to show tasks from the current project, not the entire solution. Sad... it's totally ruined the TODO-driven methodology before I had a chance to do a lecture tour.
@tarn
> background processes running the unit tests
Yep, fantastic idea.
I think that in the many-core era there's gonna be a lot of background verification.
@Vegar:
Delphi Prism's colon operator -- fascinating. But is it a good thing or a bad thing?
'OJ' on Fri, 20 Mar 2009 00:44:35 GMT, sez: @snagy: Don't flatter yourself ;)
@everyone: What about empty catch blocks? Locking on objects that are accessible to the entire process?
'lb' on Fri, 20 Mar 2009 01:42:44 GMT, sez: @OJ
>empty catch blocks
good call OJ.
how about throwing an exception without keeping the stacktrace from the caught exception.
'RW' on Fri, 20 Mar 2009 01:59:11 GMT, sez: lb,
Code that throws A 'NotImplementedException':
What about the times that it is valid? For example a method that at runtime should never get called (because it is overriden, or in a dependency injection scenario). Sure, we could do a #Pragma disable.
A parameter that is not used:
VS2008 supports this. I know, I turned it on and all the event handlers lit up :).
'Steven Nagy' on Fri, 20 Mar 2009 02:03:50 GMT, sez: @lb: I don't like your DOT one. It stops fluent expressions
myEnum.Where(x=>true).Select(y=>y).OrderBy.etc.etc.etc
'NG' on Fri, 20 Mar 2009 12:21:41 GMT, sez: Your code would suck less if… you depended less on the compiler to tell you that your code sucks.
'dolzenko' on Fri, 20 Mar 2009 20:36:23 GMT, sez: +1 for Resharper.
As I think the most points are already covered by the JetBrains guys I think they won't mind hearing your suggestion for the ones they don't have already.
'JosephCooney' on Sat, 21 Mar 2009 08:47:30 GMT, sez: I litter my codes with #if DEBUG s and write out volumous tracing information that way. I used to have a wrapper trace method and put one #if DEBUG in there to turn off tracing, but for some reason I was convinced at a later point that this wasn't optimized away by the JIT and inlining, so now I go "belt and braces" and put #if DEBUG everywhere.
'lb' on Sun, 22 Mar 2009 02:44:37 GMT, sez: @NG
>Your code would suck less if...
>you depended less on the compiler to
>tell you that your code sucks.
Care to justify that one?
Should I use unit tests to cover things that the compiler already covers?
'NG' on Fri, 27 Mar 2009 11:55:53 GMT, sez: lb,
Seeing many bad programmers over my 15+ years of development experience, I firmly believe that a cruel compiler just won’t help you become a better programmer. You do have some good points. But what you are asking for is compiler to enforce coding standards. Where do you stop then. How about down-casting without a type check? Or how about boolean expressions with constants on right side? Or how about single line if else statements without braces? Or how about empty catch blocks? List can be endless. Besides, coding standards is something good to have and follow, I used to follow them religiously and still do, but experience and wisdom does allow you to take liberty over them.
Well, it should not be compiler’s objective to make me write a better code. Compilers are not meant for that. Everyone, developers and testers included, should have desire to improve and do better. Then we will have a better quality software.
'Andrew' on Wed, 08 Apr 2009 03:20:29 GMT, sez: FYI, there's a name for not allowing "a phrase that contains more than one dot". It's the "Law of Demeter": http://en.wikipedia.org/wiki/Law_of_Demeter
'Chris' on Wed, 22 Apr 2009 20:03:21 GMT, sez: "Unused variables raise a warning, but unused parameters don't."
In Visual C++ with warning level 4 unused parameters raise a warning. In gcc "-Wunused" does the same thing. I don't know anything about C#, so apologies if this information is totally useless to you.
'Dave Kaye' on Sat, 06 Jun 2009 14:10:32 GMT, sez: OS X's XCode would have caught all of these. It's easy to say, but: there's another reason to get away from Microsoft!
|
secretGeek .:dot Nuts about dot Net:.
Just Wally
Complete secretGeek Archives
The Best Software Writing I
The Business Of Software (Eric Sink)
