- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Admin
On comments:
I have a simple methodology: Write the comments for YOURSELF, so YOU can understand the code. This is a reasonable task since if you are away from the code for about 6 months, you will have forgotten the minutia of how it works, why you did it, and the errors you discovered along the way. If you write the comments for your own understanding, they will be clear in your mind, and the side effect of this is that they will most likely be clear to others as well.
Really, a simple philosophy.
Admin
Way back in the 90's I wrote a simple toolbar app in HP's dtksh (a Korn Shell ksh93 with extensions for the common desktop environment). We had a standard tool for launching our apps but that required a full X11 environment, which our users found intrusive, as opposed to my app which launched each app as if it was hosted directly under Windows. So, of course our user base saw it and decided to switch to it. 400 users using a quick and dirty app designed for one person's personal use. The problem - there was a bug in dtksh that caused it to crash. When I put in print statements to capture data, it wouldn't crash. Finally i put in a line with
:
which is a no op that returns true. And of course I had to precede it with:
Do not remove following no op or script will fail
Admin
Exactly. There are many devs here who ding me in code reviews saying we don't need a note about this, and we especially don't need to note the Jira number, because that's what subversion is for. Except. That I worked here before we had subversion (VSS before). I may work here after we move off of subversion (git maybe?). Same story with issue tracking tools. And nobody wants to go back through 5 years of revisions. Yes there's blame, but if someone else touches the line for an unrelated reason then someone else's change shows on that line. All that to save on the 'overhead' of 2 commented lines. I don't get it.
Admin
Admin
Admin
Doesn't work. I change line 99 to address a security problem today. Someone else changes line 99 next month to add a new UI theme. Who shows up in blame? The hapless UI developer, who now gets a question about the access control there. I've been working on the same code base for 7 years. Change that 'next month' time period to 5 years. Maybe there is a good tool for finding out in what revision CheckAccessSecure was changed to FakeCheckAccessWideOpen, and if someone can point me to that I'd love it, but I don't know of one, especially since that could have moved from line 90 to line 99 in the same commit where the function name changed. But adding a comment line right above the function #Throwing this in for the XXX demo# will give a line whose ownership is significantly less likely to change, unless of course someone comes along and fixes the function and updates the comment
Admin
That appears to be C#. Every public method should have a comment for one specific reason - it shows up in Intellisense for the other developers in your company as they are using it. Given that, the other developers should come club your co-worker over the head due to the useless pop ups he's giving them.
Admin
Haha, I stand corrected. But it's always the other guy's fault, right?
Admin
Even worse is "The <see cref="Name"/> of this <see cref="Person"/>."
Intillisense pop-ups are useless in 99% of cases I encounter. Thank god it can be turned off.
Admin
Lines such as above
"# Do not remove following no op or script will fail"
are ok, except for the explanation, which might in this case be
Because of a reproducable bug in dtksh version x.y
Similarily for the likes of "Don't know why this call has to be used twice" which should be explained such as
But only one call is known to not set the given values. or Because I was told to do it that way by my boss.
Admin
Admin
Get your developers to stop using GhostDoc then ;)
These look a lot like GhostDoc's defaults!
Admin
Best comment I was ever responsible for had to do with branching to two different decode routines depending on what type of terminal (and hence what format) a message had come from. Type 3000 terminals would have IDs beginning with a '2', while the earlier Type 2000 terminals (typically in third-world countries) had IDs beginning with a '3'. You check the first character of the ID and proceed accordingly.
I could have done with flashing red text in the source code to keep newbie contract programmers from "correcting" the "obvious mistake" and sending messages from terminals starting with '2' to the Type 2000 decode routine as it should "clearly be" instead of the logical criss-cross forced upon us by whoever assigned the terminal IDs in the first place.
Admin
Admin
Maybe INFERNO is what happened when someone touched the line of code that used to be there?
Admin
You can blame Microsoft for that. Their standards state that every method and property in C# has to have a summary, and if you happen to use StyleCop then every method and property that doesn't have a summary will get a warning.
This looks like someone is using GhostDoc to follow the standards, which is fine most of the time, but they're too lazy to go back and fix the comments. Most likely they just installed GhostDoc, left it on the default settings, and then they just run it whenever StyleCop complains to make it shut up.
Admin
A long time ago (when I was green and stupid - now I'm yellow and stupid) I was tasked with writing a test harness for our product API. I was new to the product and didn't truly understand it but persevered nonetheless. After much head scratching and late nights I wrote a comment along the lines of:
"Exception thrown - did you use the wrong jobtype" or something similar, as a sanity test for myself.
7 years later I visit our customers on integration engagements and they proudly show me their web service client applications code which have this comment verbatim in there (as we distribute the test harness with the product)
I 'might' go back and remove the comment but it must make sense to lots of people otherwise they wouldn't be including it in their own codebases, right?
Admin
But where are the unicorns?
Admin
What a shame. You should have deleted all of the try/catch and implemented unhandled exception logging in a centralized location.
Murphy's law suggests that there will be a critical bug in the one method where you didn't have a try/catch with logging.
Admin
Yeah, I did put exception reporting in the Application_Error event of the Global.asax file.
I also did a Find in Files for "End Try", came in pretty handy. The app was't large enough for there to be more than a couple hundred. A little time-consuming, but well worth the effort.
Admin
I found something like this once:
Gee, thanks. There is no way in hell I would have figured that out in a million years without this immensely useful comment.
Admin
Comments are what makes this job interesting.
Admin
Admin
Long, long ago, I was reading the source code listing for the Data General Eclipse AOS kernel, a 16 bit minicomputer.
The kernel had a 16 bit status word indicating the state of various important things in the bits. The hardware had general bit testing and setting instructions but also had two special fast instructions for testing the sign bit and the bit next to the sign bit.
The kernel defined constants for the various bits, something like:
etc.
Thus, you could use the constant in the general bit manipulation instructions, but not for the fast instructions, which would reference bits 14 or 15 by context. Thus, if you changed the location of TTIS or TTOS in the word, the code that was optimized for the quick instructions would break. Immediately above the constant definitions was the comment:
-- Charles
Admin
Similar techniques have been coded, to replace a comment with a piece of functional code enabled under certain circs (did it myself in a web.xml file designed to be compatible with both a tomcat4 and tomcat5 environment, IIRC.
The trick is to include somewhere in the vicinity a comment which actually explains what you are doing. So, not necessarily a WTF, but without that explanatory comment, fucking stupid all the same.
Admin
No, just means it never gets triggered and nobody ever notices it.
Admin
Frustrating though it is, I much prefer over commented code than undercommented code eg:
/* Declare integer i */ int i;
/* Set i to 1 */ i=1;
Yes it's stupid and annoying, but I find this far easier to wade through than something with NO comments....
Admin
Admin
Anyone who believes code can always be refactored to be readable without comments is kidding themselves. Comments aren't necessarily needed all the time, but there is always a situation where logic is complex enough to require some explanation - why this complexity exists, what it is doing, and perhaps why this approach was taken.....
Anyone who believes NO COMMENT is a good thing (or worse yet that it is possible to write easily supportable code without ever writing a comment has obviously never had to look after code from a third party.
Admin
Not sure you need to delimit the end, necessarily, but I have no issue with: //C453674 - Calculate GST before processing GST free items.
Admin
And the other 0.2:
Admin
If the ORIGINAL AUTHOR is saying "why is this here" you have a bit of a problem (even if they added that on a revisit) If a SUBSEQUENT SUPPORT PERSON is adding that comment, then it serves as a reminder that, once we have time we probably should go nut that out......
I recently found one (in C code) that said something akin to:
I was a bit worried that they avoid fixing something lest they break something - surely that is a risk in anything we do. I suspect that the Business refused to let them fix it (they're always afraid the whole world will collapse ig we correct something), and I think that should have been made more clear in the comment....
never mind.
Admin
Admin
Admin
Admin
I am aghast reading through most of these comments, because it immediately becomes clear that many people here have (or think they have) worked in development rather than support (although I suspect for many of you, "worked" = written a Tic Tac Toe game for my little sister using drag and drop technologies).
Comments explain what is going on. Even the most fluent programmer in any language won't immediately understand everything about a piece of code no matter how well/self-documenting you think you wrote it. Of course, given time, many could probably nut out what's going on - but the longer it takes them to fix an issue, the more they will curse (to themselves, to their peers and to the business) the "numpties" that created the unmaintainable spaghetti they have inherited. In some respects, you may not care because you've moved on, but surely your pride dictates that:
It's no fine art, but if you sometimes revisit stuff you've written (like way back when) you start to get a feel for whoch of your comments were good, and which are stupid - and over time, you can refine your behaviour to generally write good comments when they're appropriate and leave the crap ones out. Of course, none of us can ever claim to have perfectly coded or commented something - because what seems trivial today might be of utmost importance tomorrow.
Admin
followed by some code that used the variable instead of the literal. And yes, it did work around a compiler bug.
Admin
It started at 'varsity. I had a lecturer complain that code was insufficiently commented (TBH he had a point, but that's hardly the point here) and I asked what we expected. Naturally that was a bit of a difficult question for him to answer, so I commented EVERYTHING...the habit stuck for my first few major projects in "The Real World" (tm).
Admin
Admin
If you don't understand the business logic, you have no business implementing the logic. Business logic and policy are distinct. "Business logic" is the configurable portin
What is the "same reason why..."? You aren't making any sense. You say you don't need to understand the policy that lead to a plan, and then say the use of domain specific variable names is induced by "the same reason".
Admin
Well, this is my only comment on the comment on the comment on the comment on the comment. There, I've just said what but not why. /* I don't know why I did that. */
Admin
You realise you can actually (gasp) modify the settings of stylecop and turn off warnings your team thinks are not necessary.
Admin
http://blogs.msdn.com/b/larryosterman/archive/2005/03/29/403439.aspx
By the way, does anyone know why Akismet is here?
Admin
Except when one of your business metrics for 'good quality code' is 'StyleCop under XXX settings returns no warnings'. Its far easier to write dumb comments than convince the head of development to change metrics.
Admin
/* Best comment yet. */
Admin
/**
I wonder if your version control system has an older version of this that DID have "advanced" code in it, but another developer came along and stripped everything but the comment and returning true...
What if they did it to leave as a clue for some sleuth to come along and find, digging through the history so that they may discover utter sorrow, pain, and misery along their forsaken way?
If the OP would be so kind as to check, it could add to the beauty of the WTF.
Hopefully I'm not getting my hopes up!
Admin
/* Agreed. */
Admin
Let's refactor the good Sgt's first sentence and remove the double negative...
"...does doesn't... don't...." should become positive "does" for something like "Just becuase I can figure out what a block of code does, I should understand the business logic" But that sounds weird. Refactoring a bit more, "Even if I can figure out what a block of code does, I should understand the the business logic." Kinda matches your point.
Sgt, please avoid double negatives. It helps communication.
Cpt, please note double negatives and engage your secondary lexical parser. It's less performant but working slightly harder to understand others helps communication.
Communication means we can address more interesting problems, like why TDWTF stories have been going downhill for so long.
Admin
But that's not to say another version might be both clearer and come across the right way.
(And the double-negatives has implications for coding style too -- in particular, I find it rather easier to understand Booleans that are named in the positive sense. I've worked on a code base that's essentially an emulator and contained a field called isNotHalted... and you can only see if(!isNotHalted) so many times before you want to get a bit stabby and wish it had been called isRunning. :-))