- 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
The reason to put comments is not to echo the code. It's to EXPLAIN what the code is doing. Not so much WHAT, but WHY... requiring a comment-to-code ratio misses the point.
Comments should make the code transparent to the maintainer (even if the maintainer is the same as the author.)
In this particular example, all the comments are wasted keystrokes. It would have been better if the coder would have provided some examples of how the function would be called, why serialization was necessary, what kind of exceptions would cause the try to fail, etc.
I'd say the WTF is the coder's attitude.
Admin
Maybe your friend should learn how to write comments. What preconditions are there to the method? What are the requirements for the parameters of the method, can you pass in null? Can you pass any string? An empty string even? In what conditions will there be an exception thrown? Are there special error codes for the return value? Does the method has any side effects? What conditions will the return value fulfill? I should be able to read all that from the comments preceding the method, not by deciphering the code it self.
Admin
Also funny that many of the comments are very wrong. Setting something to null is not what I would call "instantiating". And the code doesn't "<font color="#008000"><font face="Courier New" size="2">set it equal to the entity object</font></font>", it is assigns [the result to] entity.
Admin
Also, since this is obviously C#, why not use the standard built-in C# / VS.NET comments? You know, <summary>, <returns>, it's there for a reason!
And yes, I'd say most comments could be more usefull, if you are asked for more comments, add usefull comments! What possible exceptions can occur? What values for input are allowed, what precisely does it return?<FONT face="Courier New" color=#008000 size=2>
</FONT>Admin
Agreed that the WTF is the coder's attitude. I think the ancillary WTF is why the boss is reviewing the code and not a peer. I see some things in there that I think are questionable. However, thank you for sharing.
Admin
That's no so bad, I'm currently commenting to almost that depth right now. However the code is part of a mentoring process, and I want them to know exactly what it does once I'm gone and follow it through (it's some nifty reflection code, a custom IDataReader and a custom set of identity and principals)
As it's a library that they'll be plugging into and not looking at (hopefully) until they need to understand it the insane commenting level is very deliberate.
Admin
Sometimes I wind up with unnecessary "//initialize objects" -style comments in my code because when I sit down to write a function, I'll often "sketch" the function out in pseudocode comments before writing the code.
So occaisionally some silly comments make it into production code. Then I get teased about them. "Thanks, dude... I wouldn't have guessed that 'dim counter int' initialized a counter variable. Good thing you commented that!"
(-:
Admin
Except those comment decorations are targetted at producing documentation about a class or method, not explaining the inner workings.
Admin
This presupposes that the comments were added to the code, which ain't necessarily the case. A common (best) practice is to write natural language pseudocode first (as comments, to avoid flinging errors around), then add working code to the pseudocode. If that were the case here, the real problem is that the pseudocode is too close to the programming language -- it reflects how the code will be written rather than what the code will be doing. The .NET <items> are definitely post-coding comments, much like JavaDoc comment tags, and are much more focussed on the API-ness of the class than on the methodology.
Admin
<FONT face="Courier New" size=2>This code:</FONT>
<FONT face="Courier New" size=2>if (log.IsInfoEnabled) log.Info("Reading in XML");</FONT>
This is a WTF. Why doesn't the Info() method check if IsInfoEnabled?
WTF!!!!!!!! [:'(]
Admin
Apparently this coder had a serious 'tude that day.
He was obviously rebelling against management and everything they stood for.
Admin
It is the case here though - it's stated in the text of the post that the programmer was asked to add more comments by his boss.
I agree 100% with writing natural language pseudocode first, I do this for any method that I write that's going to be more than a few lines of code.
Admin
Well, the boss's take on the comments sounds an awful lot like C. Montgomery Burns telling Don Mattingly to "shave those sideburns, hippie!"
"I don't know what you think comments are, but..."
Admin
lol - <!--StartFragment --> Mattingly: "Whatever...*muttering to himself as he walks away*...still like him better than Steinbrenner."
Admin
It probably does. However, the check before calling the log.info() method can save a lot of time when logging is disabled, and when the arguments passed to the logger are relative compex (e.g., string concatenations, casting, etc.). So, for that reason Log4J promotes this style of log-statements. (In this case, when just logging a single string, the difference will not be that large, but for consistancy, I think that you should just stick with checking whether logging is enabled.)
Admin
In our C# code we've got lots of <summary></summary> tags attached to methods, properties, enumerations, and variables that are pretty much self-explanatory.
Why?
Because in VS.Net (the original version, the one we use here) there's no way to selectively shut off the warning about not commenting elements that you'd want to have described in documentation or in intellisense. So you either have to turn off the warning entirely, or comment everything in your code. Ugh.
Admin
Then make log.Info a conditional method. The compiler will remove the entire call when logging is disabled.
Admin
Re: "Then make log.Info a conditional method. The compiler will remove the entire call when logging is disabled."
That's great when you know if logging is enabled or not at compile time. If it's in a config file, and can change at runtime, this approach would not work.
Admin
WTF, he forgot a couple lines...
<FONT size=2><FONT face="Courier New"> </FONT><FONT face="Courier New" color=#0000ff>finally
</FONT></FONT><FONT face="Courier New" size=2> {
<FONT color=#008000>//Check if reader is not null</FONT></FONT><FONT face="Courier New" size=2>
</FONT><FONT size=2><FONT face="Courier New"><FONT color=#0000ff><FONT color=#000000> </FONT>if</FONT> (reader != <FONT color=#0000ff>null</FONT>)
<FONT color=#008000>//Close the reader</FONT>
</FONT></FONT><FONT face="Courier New"><FONT size=2> reader.Close();
</FONT><FONT size=2> }</FONT></FONT>
<FONT size=2>Thats much better now.
</FONT>
Admin
Actually he did include the xml comments, I didn't see fit to post them as well. He does understand the concept of comments just fine. His boss is the WTF. (I used to work there, I understand his situation very well). It's why I left.
Admin
Reminds me of something I'm always going through in my job. Even the most obvious, self-documenting code needs more comments, always. And sometimes a request is made for more comments on "tricky algorithms" like bitshifting and masking, where basically the only comments possible would be long-winded explanations of how bitshifting and masking work, and/or just spelling out code like
mask = 1 << pos;
data[slot] = (data[slot] & ~mask) | (val << pos);
with comments like
/* Take the current data in the slot, do a bitwise AND with all of the bits which
don't correspond with the position, then do a bitwise OR with the new value
shifted into the position
*/
Admin
As the person responsible for the comments I'd like to chime in.
I work in a shop where the decision was recently made to migrate all of our existing web applications to .Net and this is one of the first efforts.
I think the part of the original post that is getting overlooked is that this had to be redone three times in order to satisfy the manager. The comments that you see are what the manager wanted. The only thing I could figure is that since they weren't familiar with C# syntax they couldn't read or understand the code without the comments being written that way.
But some of the responses give make me think WTF???
Like the one about the incorrect usage of the term instantiate, when you create a variable by giving an object type a name you are creating an instance of that object, thus, instantiation. Initializing the new object is when the value is assigned.
Also I can verify that the logger being used is log4net and it does indeed reference the config file to see what the logging level is set to.
Sometimes you have to do things that are absolutely ludicrous to please somebody in power.
Admin
No. It creates a new variable, which refers to the null object. No object has been instantiated. There is no constructor called. Variables are not objects. Variables refer to objects.
Admin
Why does the manager want to read the code? Insane in the membrane
Admin
omg. OWNED!
Admin
You're an idiot dude. Seriously WTF? If Log.Info check to see if it was enabled on every call you would be making tons of unnecessary checks (and potentially creating unnecessary locks if the logger is thread safe).
Admin
He's checking log.IsInfoEnabled (which references the log object) before every call to log.Info anyway. Since it's the same object, the Info method can check the flag internally and save an object reference. It would also make the code look a little cleaner.
Admin
Well the main reason that you call a check procedure before doing the logging is to save you having to construct the arguments of the log call if you don't need to.. I'm sure that doing string concats are going to cost more than one reference and method invocation.
Admin
Well, yea, there is that... [:P]
Admin
I had the opposite 'boss problem' at my first programming job out college; I was commenting some code explaining a moderately complex code change (Windows 3.1 API code in 'C'-- dealing with a structure containing lots of handles) when a 'senior' programmer walked by and saw what I was doing.
He chastized me for writing comments-- and twelve years later I can still quote him verbatim: "Real programmers don't need comments." (strongly implying that I was not a real programmer) "Real programmers can just look at the code and understand what's going on."
He then insisted that I remove all comments, and stood over my shoulder to ensure that I deleted all of my comments. He walked away, satisfied from his power trip of belittling a kid just out of college.
Admin
I agree in some respects and disagree in others. Your comments leave much to be desired in explaining the code. For instance when you declare a variable, I don't expect to see a comment saying "Declaring a variable". ALMOST anyone can see that for himself. Surely more useful would be a comment saying what the variable is used for? eg for the object entity = null, I would have thought "object to store the deserialized xml to" would be more appropriate.
If this IS what the manager wanted, then yes the blame belongs to him. But if he/she asked for a comment on the variable declaration and you gave him THIS, I believe some of it falls to you.
Just my 2 cents.
Admin
This one is hilarious:
<font color="#008000"><font face="Courier New" size="2">//instantiate and initialize a string reader object
</font></font><font face="Courier New" size="2">StringReader reader = <font color="#0000ff">null</font>;
How much can you do wrong here?
2. You don't need to assign null to a variable that is already guaranteed to be null.
3. The comment is entirely wrong about what the statement is doing. (There are no objects at all involved here, as was pointed out earlier.)
And the programmer has an excuse only for the first item on that list.
I always ask for code samples when someone is applying to me for a programming job. I actually got almost exactly the above in one sample. Needless to say, the guy's resume went straight into the trash can.
As for comments: I strongly encourage people to avoid them whenever possible. If you find yourself tempted to add a comment to something, you should first look to see if you can't rewrite the code to be clear enough that it's easily understandable without comments.
Ron Jeffries, I think it was, had a good story about reading some code for the first time from an XP project Kent Beck was working. There were almost no comments in it, but he had no trouble understanding it at all. He said that reading the code was like reading English.
</font>
Admin
Personally, I think these snippets are more irritating than the comments.
1. Putting multiple statements on one line is just a pain in the ass for the debugger. Have you ever tried to put a breakpoint on one of these lines, and as soon as you press "F10" (or whatever), the next statement is highlighted! How can you tell if the result of the conditional was evaluated? Maybe in C# you can check the debugger for variables, but if IsInfoEnabled is a function, then you're out of luck in Visual C++. Whenever I see this kind of multiple line crap, I ALWAYS reformat the code and put the result of the conditional on the next line, indented.
2 Which brings me to my next point: Poor formatting makes for hard to read code. Results of conitionals should be indented.
And finally, I'm writing this while working out on my elliptical trainer. I love wireless internet! :)
Admin
IMHO the bigger mistake is to write log.Error("text"+ex) instead of log.Error("text",ex) .
With the first code you might loose the exception stacktrace...
just my 2c
<font face="Courier New"><font size="2"></font></font>
Admin
1. I agree. Comments should go in places where they do not impede the flow of reading the code
2. Can you guarantee it will be 'null' in every implementation of a C# compiler? I think it's good practice to assign even default values to variables. Like assigning False to a newly created boolean in VB.NET. It does default to false... for now. But someone reading my code will know for SURE it starts it's life as being False.
As for comments: I think explanatory comments are fine, in some places. If I have a for/next loop which say, creates some XML, I'll put a comment in front of it saying that it puts the data into an xml structure. However, that is all I will put there. This comment is enough to let someone skip the loop if his/her interest is not in the creation of the XML, or if the interest is there, to quickly locate the place where the XML is generated.
So, IHMO (in my honest opinion) comments should be there so that people who debug can easily find relevant sections of code. Then they read the code to see what it does. If they can't read the code, they should not be debugging it.
Drak
Admin
<FONT size=2>He thanks ! I wondered what those lines were doing ...
</FONT>Admin
The biggest problem with code is always that you need to make sure the code stays readable. This often means you have to add comments just to describe what your code is about to do. I've heard from my dad that at IBM, Assembler programmers had to add a line of comment with every line of instructions, just because the language is so cryptic. They just enforced readability of the sourcecode. So my dad also learned to add lots of comments. And he told me to add lots of comments to my code too but I'm a bit too lazy for that. [:$]
Still, he is right. He can view his projects from 4 years ago and the code is very clear about how it works. Half of it is documentation anyway. I can't barely understand things I've written half a year ago, just because most of the comments are missing... But okay, I'm learning, okay? Just have to get used to it, that's all.
My dad did create a project once for some company, and when he delivered them the source, they complained about the large amount of comments in it. He was asked to remove it but he just refused and told them they would be sorry if it was removed. They didn't listen so when one of their programmers took over the project, the first thing he had to do was remove all the comments and it seemed my dad would not be hired again by them. But a year later the company contacted him again because apparantly the project was maintained by their developers but they just could not understand how it worked so they messed around with it. It had turned from a nice, working system to a slow, sluggish behemoth that would crash at unexpected moments. If my dad would be so nice to help them fix it... A very expensive question for them, since my dad just had to use his copy of the original source and implement all changes they wanted to have added to it. He charged them triple his normal rates too, just as a simple lesson for them to be careful when you mess with someone's code and ignore the comments...
That same year we also moved to a much bigger house... [:D]
It is weird though, but at school they hardly ever talk about adding comments to the sourcecode. They do talk about maintaining additional documentation that describes the code (technical design stuff) but not really about comments itself. In a language like Pascal or VB I can understand that comments aren't always needed because the language is close to a normal written language but C++ and Java are a bit more cryptic, thus a bit harder to read...
Admin
Assembly is cryptic? If you evaluate code line by line, that's the least cryptic language I'm aware of. I see no point in adding comments for every line of code since a single line of assembly doesn't usually move mountains.
Of course comments for several lines of code (which actually "does" something) are very very useful.
Admin
Wrong. If you do not explicitly assign null to a variable the C# compiler might return some errors, like 'Error: use of unassigned variable 'reader' in like 523', although granted, this generally counts more for primitive types than other objects.
Admin
<FONT style="BACKGROUND-COLOR: #efefef">It's better to have code with less comments then to have code with lot's of comments which are out of date ;). And comments like in this post just makes it a pain to read the code.</FONT>
<FONT style="BACKGROUND-COLOR: #efefef">Guess that to learn to write code is easier then writing good comments.</FONT>
<FONT style="BACKGROUND-COLOR: #efefef">Katja, I wonder how that IBM asm code looks like.. something like this? [:P]</FONT>
Admin
I'm sorry but I think that your comment is not polite and also not correct. Anyway the info should be written only if the Info is enabled so you have to do that checks anyway, and it's better that checks to be done by the class and not by the caller. Try not to be rude and think before you post something...
Admin
That's a great story!
That is weird, and a WTF too IMHO. Why wouldn't a school teach commenting? I would think that great emphasis should be placed on commenting and formatting code when teaching ANY programming language. At school we would lose lots of marks for not indenting or commenting our code.
Admin
Actually for this example a comment could be useful:
/* Store the value into its appropriate place
*/
mask = 1 << pos;
data[slot] = (data[slot] & ~mask) | (val << pos);
Maintenance programmers will thank you.
Admin
...sorry about the '< br>' growls at the previewer
Admin
Actually I sort of agree with your senior programmer: Well written code should not need inline comments. Key words here being well written and inline. What code needs are correct and useful names for variables and methods and methods should not do too much. Too much code I see have methods that are too long with vague names where comments are used trying to solve the lack of overview and clarity.
Commenting why's as some people suggested has similar problems. It represents a choice made by the developer. If this choice affects behaviour it should be documented as the behaviour in the api docs otherwise it does not matter, the choice is encapsulated in a method and if an other developer chooses to replace it with his or hers version or choice it should have zero impact. Unit test should verify this of cause.
I love things like javadoc and ndoc and expect them to be used extensively , but inline comments to me usually point out design flaws.<o:p></o:p>
RC
Admin
Yes, inline comments are a bit of a pain -- when the code is trivial or does not innovate in any way. The moment you move away from the obvious, even the best-named variables and methods will fail to convey quite what's going on. Sure, you've done a two-bit left shuffle -- that much is obvious. What did you accomplish by doing that? Can a maintenance programmer figure out why you did it, or whether or not it's safe to play with the two lines following that one?
Unsyncronized inline comments are bad, but not nearly as bad as uncommented code with missing doco. And no, API-level comments (JavaDoc and the like) are no help at all to anyone looking at the inside of a module. It doesn't help at all to know what the inputs and outputs of the black box are supposed to look like when what you are trying to fix/change lives inside the black box. To put it in hardware terms, knowing that AC power comes in here, a microphone plugs in over there, and that thingy is where the loudspeaker connects does nothing at all to help me fix the amplifier.
Admin
Amen.
I did however insist on adding a comment the other day.
There was code like this
theObject.Update();
//this looks stupid, so I explain why it is needed.
theObject.Refresh();
//Even with the comment you'll say WTF, but thats better than it crapping out
dosomething(theObject);
theObject.Update();
Admin
If I coded like this, it would need comments
using System;
public class TheRing : Oroboros
{
public TheRing(Oroboros oroboros): base(oroboros){}
new static public TheRing New()
{
return Oroboros.New() as TheRing;
}
}
public class Oroboros : System.Configuration.IConfigurationSectionHandler
{
string _tail;
public Oroboros(){}
public Oroboros(Oroboros oroboros){}
public static Oroboros New()
{
Oroboros config = System.Configuration.ConfigurationSettings.GetConfig("Oroboros") as Oroboros;
return Type.GetType(config.Tail).GetConstructor(new Type[1]{typeof(Oroboros)}).Invoke(new object[1]{config}) as Oroboros;
}
public string Tail
{
get
{
return _tail;
}
set
{
_tail = value;
}
}
public virtual string Name
{
get
{
return this.GetType().Name;
}
}
public object Create(object parent,object configContext,System.Xml.XmlNode section)
{
return new System.Xml.Serialization.XmlSerializer (Type.GetType ( section.Attributes["type"].Value )).Deserialize( new System.Xml.XmlNodeReader (section));
}
}
Admin
Duh, it would of course look more like this:
Now my x86ASM is rather underdeveloped so I'm not entirly sure which way the MOV works (source, dest or dest, source), but I think this would be a clearer comment..
I used a lot of comments in the 2 or 3 68k ASM programs I built years ago, I think I should still be able to figure them out now, after all this time.
Drak
Admin
I comment my code the same way, because I want to and if you laughed at me for it, I'd promptly explain to you just how stupid you were for making a big deal out of nothing.
It seems like the real WTFs are starting to evaporate and much like the elitist Linux crowd it's turned to bashing people for the sake of boosting ones self esteem.
The only WTF here is the thread.
Paul