Commenting your code

RockLobster · 8 Nov 2007 at 22:55

I'm going to be working on some fairly big projects soon and one of the things that popped into my mind what is the best method for commenting. Full detailed header at the top of the file / page? For C# developers out there do you use specially marked comments to produce XML files?

What brought me onto this is a guy in work has gone off sick, it could be long term. I was asked to fix a few bugs in one of his apps, none of the code was commented! The variable names weren't great either. It got to the point I just could not work with the code so we have had to pull the project until he comes back. It made me look like a fool and managers, etc don't understand the problems it can cause by him not commenting.

It also made me realise how important is to write good comments are use descriptive variables, etc.

What do you think?

stx666 · 8 Nov 2007 at 23:11

Good question!

Generally I comment very little in terms of code unless it's complicated, as most refactored code will tell the story.

Tend to put a brief description above methods, so you can read the description in the intellisense as below.

/// <summary>
/// test method
/// </summary>
/// <param name="t1"></param>
private void test(int t1)
{
}

chesterstu · 8 Nov 2007 at 23:16

I generally tend to comment everything with the /// style ones at the top of methods. But the main thing is giving description var names and methods names as you can figure ou what is ging on from them normally

Inquisitor · 8 Nov 2007 at 23:19

I try to comment my code only where it's needed. Well-written code shouldn't need comments everywhere; only where it's not immediately obvious what a block of code does, or if there's some non-obvious problem you're catering for.

Of course, descriptive variable names are an absolute must and there's no excuse not to use them

As for method-level documentation, I pretty much always use this to fully comment all of my methods, types, properties, etc.

Shoseki · 8 Nov 2007 at 23:25

Well written code, with well chosen class/variable names and simplified methods doesn't require a ton of commenting, I prefer light // based comments, comment blocks for things like licensing at the beginning of the files, explanations for things that are really obscure.

Some IDEs go overboard and put text blocks over every method, not my kind of fun. well named (and predictable methods) are the best way ahead.

EDIT : For functions that do something unusual, a good comment goes a long way. But you don't have to highlight something you can get just by reading the code.

If you took the code/algorithm from somewhere else, it is kind courtesy to reference it (either in the code or documentation) or else if discovered it could be plagiarism/copyright theft.

Haircut · 9 Nov 2007 at 00:26

Well written code shouldn't require a lot of commenting saying what it is doing.
The value added comes from when you say what you are doing it for.

Even the most well written code in the world should tell you what its purpose is

voodooflux · 9 Nov 2007 at 12:15

To reiterate what most people have said, the best code should require minimal comments. I often see the most obvious things commented, such as:

Code:

[COLOR="YellowGreen"]// Save the order[/COLOR]
order.Save();

These type of comments are pointless and do nothing but add clutter.

I make significant use of the /// style comments in C# as these add useful detail to the intellisense information displayed and allow for generation of documentation. I tend to only include the summary for private/protected methods...

Code:

[COLOR="Silver"]/// <summary>[/COLOR]
[COLOR="silver"]///[/COLOR] [COLOR="YellowGreen"]This method does something private.[/COLOR][COLOR="silver"]
/// </summary>
[/COLOR]private void DoSomethingPrivate(int something)
{
...
}

...but add as much useful information as possible for public methods...

Code:

[COLOR="Silver"]/// <summary>[/COLOR]
[COLOR="silver"]///[/COLOR] [COLOR="YellowGreen"]This method does something public.[/COLOR]
[COLOR="silver"]/// </summary>[/COLOR]
[COLOR="silver"]/// <param name="something">[COLOR="YellowGreen"]A something parameter[/COLOR]</param>[/COLOR]
[COLOR="silver"]/// <returns>[COLOR="YellowGreen"]The number of something returned[/COLOR]</returns>[/COLOR]
public int DoSomethingPublic(int something)
{
...
}

Task List Comments are also handy in Visual Studio, such as...

Code:

[COLOR="YellowGreen"]// TODO: obtain the connection string from the config 
// file rather than hard coding it.[/COLOR]

...as these show up in the Visual Studio Task List. You can add custom task list comment types as well.

On a similar vein I like to use "arse-covering" comments like...

Code:

[COLOR="YellowGreen"]// ASSUMPTION: Agreed this with <name of business analyst> during 
// requirements review meeting on 08/11/07.[/COLOR]

SiriusB · 9 Nov 2007 at 12:57

I usually comment for my own benefit, especially if a particular method/chunk of code is something I haven't done before or is unusual. I tend to forget what the hell I was doing even after just a few weeks away from a particular project and my own comments really do help out. Once I am happy with my code I remove any comments that aren't really needed.

As said, as long as your variable names are very concise you wont need a lot of commenting.

RobH · 9 Nov 2007 at 13:40

I am a Java developer and I tend to make use of Javadoc wherever possible. It does help as you can go back to a chunk of code and think "did I write this?" and "what the hell does it do?" not to mention if other people should dare to read it. In most programming languages it makes sense to comment what subprograms (methods, functions, subroutines) do as you'll likely be using them in many places if you designed you code well.

Inquisitor · 9 Nov 2007 at 16:22

voodooflux said:

I tend to only include the summary for private/protected methods...

Code:

[COLOR="Silver"]/// <summary>[/COLOR]
[COLOR="silver"]///[/COLOR] [COLOR="YellowGreen"]This method does something private.[/COLOR][COLOR="silver"]
/// </summary>
[/COLOR]private void DoSomethingPrivate(int something)
{
...
}

...but add as much useful information as possible for public methods...

Code:

[COLOR="Silver"]/// <summary>[/COLOR]
[COLOR="silver"]///[/COLOR] [COLOR="YellowGreen"]This method does something public.[/COLOR]
[COLOR="silver"]/// </summary>[/COLOR]
[COLOR="silver"]/// <param name="something">[COLOR="YellowGreen"]A something parameter[/COLOR]</param>[/COLOR]
[COLOR="silver"]/// <returns>[COLOR="YellowGreen"]The number of something returned[/COLOR]</returns>[/COLOR]
public int DoSomethingPublic(int something)
{
...
}

Remember that protected members are just another type of public member, as they are visible outside the assembly

It's not really relevant whether they're visible outside their parent type or not; it's whether they're visible outside the assembly that matters. After all, if you have access to internal members, then you also have access to all private members in the assembly (via the source).

Dj_Jestar · 9 Nov 2007 at 17:10

JavaDoc/PHPDoc is about the most extensive I've ever commented.. I prefer to let my code do the talking.

Sic · 9 Nov 2007 at 17:36

Dj_Jestar said:
I prefer to let my code do the talking.

to a certain degree, I'd definitely agree with that, but sometimes I go back to things and I'm like "what was I smoking when I wrote that?!?!", so I do tend to do a bit of commenting around parts that I think are going to bend my mind later. If it's something that's quite involved and can be used elsewhere, I might write an example of usage as a mini guide on how to use it.

voodooflux · 9 Nov 2007 at 18:44

Inquisitor said:
Remember that protected members are just another type of public member, as they are visible outside the assembly

It's not really relevant whether they're visible outside their parent type or not; it's whether they're visible outside the assembly that matters. After all, if you have access to internal members, then you also have access to all private members in the assembly (via the source).

I understand what you're getting at there, I just consider the public interface to be the API (contract) that needs to be documented when developing a layered system rather than a class library that is designed to be extended (interface vs. inheritance). You're right that protected members can benefit from additional documentation where they are used, and where the class has been designed to be extended.

Inquisitor · 9 Nov 2007 at 18:46

voodooflux said:
I understand what you're getting at there, I just consider the public interface to be the API (contract) that needs to be documented when developing a layered system rather than a class library that is designed to be extended (interface vs. inheritance). You're right that protected members can benefit from additional documentation where they are used, and where the class has been designed to be extended.

That's just it though: protected members are part of the API's public interface. The very fact that a class has protected members means that it has been designed to be extended.

voodooflux · 9 Nov 2007 at 18:48

Inquisitor said:
That's just it though: protected members are part of the API's public interface. The very fact that a class has protected members means that it has been designed to be extended.

I agree with you - I was just trying to say (albeit not very well) that when programming to a contract/interface I don't tend to use protected members anyway, as those systems aren't designed to be extended. Class libraries on the other hand often do provide for extension via inheritence, and the protected members are part of the API in that case.

Inquisitor said:
The very fact that a class has protected members means that it has been designed to be extended.

It would be nice to think that

Shoseki · 9 Nov 2007 at 23:13

Sometimes, though, certain things like making things protected/private are to do with design patterns rather than pure OO programming.

OpenSG, for example, makes heavy use of the prototype paradigm, its not actually possible to call NodePtr n = new NodePtr() because it is private - you have to call NodePtr n = Node::create();

Inquisitor · 9 Nov 2007 at 23:18

voodooflux said:
It would be nice to think that

Why would there be protected members if it hadn't?

voodooflux · 9 Nov 2007 at 23:57

Inquisitor said:
Why would there be protected members if it hadn't?

Have you never encountered a class where the developer has marked some methods as protected, but the class itself has not been designed for inheritance? Making a class suitable for inheritance isn't always a matter of just slapping the protected keyword onto a few methods, and can require careful design.

Inquisitor · 10 Nov 2007 at 00:09

voodooflux said:
Have you never encountered a class where the developer has marked some methods as protected, but the class itself has not been designed for inheritance? Making a class suitable for inheritance isn't always a matter of just slapping the protected keyword onto a few methods, and can require careful design.

Perhaps not carefully designed, but if there are protected members there then it's a pretty safe bet that the developer has put them there with the intention of making the class extendible. That's their purpose, after all.

voodooflux · 10 Nov 2007 at 00:11

Inquisitor said:
Perhaps not carefully designed, but if there are protected members there then it's a pretty safe bet that the developer has put them there with the intention of making the class extendible.

Indeed one would hope so. A good quote from Joshua Block (he of "Effective Java") goes something along the lines of "when designing a class it should either be designed for inheritence or prohibit it." Experience just demonstrates that while the presence of protected methods *should* indicate that a class can be inherited, it isn't always a safe bet. Just me being a pessimist.