18 replies to this topic

[xorangekiller]

I know that commenting is a huge debate in programming. I was reading this article on Ars Technica, which is not primarily about commenting, but noticed that the person asking the question noted that he doesn't comment his code because he believes it is self-evident. However, most professional software development firms require commenting of some sort. So what is your personal opinion on commenting your code? Do you comment a lot, not at all, or somewhere inbetween? And, most importantly, why do you take that position?

As I have become a more experienced developer I have developed my own style. By most programmer's standards I believe that I comment quite a bit. I have a couple snipets of my code below, with the full source code for the class attached. (Note: The spacing is slightly screwed up. Not my fault. I blame Neowin's post editor.)

/*
xorangekiller's File System class for easy manipulation of objects on the file system.

This class is essentially a wrapper for Windows API functions.
However, unlike the file manipulation functions in the Windows API, all of these work recursively.

Windows file management functions: http://msdn.microsoft.com/en-us/library/aa364232.aspx
Windows directory management functions: http://msdn.microsoft.com/en-us/library/aa363950.aspx
*/
class xFileSystem
{
  public:
	// Initialization
	xFileSystem();
	virtual ~xFileSystem() {};
	// Primary File System Utilities
	bool CreateDir( const std::string File );
	bool Copy( const std::string Source, const std::string Dest );
	bool Move( const std::string Source, const std::string Dest );
	bool Search( const std::string File, std::string Search = "", bool Case = true, bool Hidden = false, unsigned int Recurse = 0 );
	bool Delete( const std::string File );
	// Support File System Utilities
	static bool Exists( const std::string File );
	static bool IsFile( const std::string File );
	static bool IsDir( const std::string File );
	unsigned long long Size( const std::string File );

/*
Search for files directories.

Callback:
  Every time a matching file or directory is found, SearchCallback() is called.
  If it is not overridden, it will save the found files to internal storage, which can be accessed by calling GetFoundFiles() after this function returns.

64-bit Note:
    If this function is being run from a 32-bit program on 64-bit Windows you might need to call Wow64DisableWow64FsRedirection() first to allow searching 64-bit paths.
    If you use the aforementioned function, don't forget to call Wow64RevertWow64FsRedirection() after it completes to revert the setting!
    MSDN Documentation: http://msdn.microsoft.com/en-us/library/aa365743.aspx
    
Side Effects:
    Be advised: If you are using the default implementation of SearchCallback(),
    the vector GetFoundFiles() returns is reset (cleared) every time this function is called.

Arguments:
    File [in]				   Directory path to search.
							    If you supply a file we WILL return with an error!
    Search [in] [optional]	  Find files and directories matching this search string.
							    Wild characters '*' (any number of characters) and '?' (only one character) are supported.
							    If the parameter is left blank everything we find will be processed!
    Case [in] [optional]	    Is the search string case sensitive?
							    This paramter is ignored if Search is "" or "*".
    Hidden [in] [optional]	  Should the search include hidden files and directories?
    Recurse [in] [optional]	 Depth to enumerate the search path.
								    0	   Enumerate the entire path.
								    1	   Enumerate only the base directory.
								    2	   Enumerate the base directory and any directories it contains.
								    3+	  Enumerate all directories up to the designated level.

Return Value:
    true	    We found at least one file or directory matching the search parameters.
    flase	   Nothing was found matching the search parameters.
			    Are you sure we have permission to access the path?
*/
bool xFileSystem::Search( const std::string File, std::string Search, bool Case, bool Hidden, unsigned int Recurse )
{
  //
  // Check for trouble.
  //
  if( !this->IsDir( File ) ) return false; // We can only search directories!
  if( Search.empty() ) Search = "*"; // If the search string does not exist, search for all files.
  this->FoundFiles.clear(); // Clear our found files vector.
  //
  // Search the source directory.
  //
  xDirIt Handle( File, true, Hidden, Recurse ); // Handle for iterating the directory tree.
  while( Handle.It() )
  {
    if( this->Abort ) return false;
    // Strip the full directory path.
    const char * FileIt = Handle.FileName; // Pointer to the file name that we will iterate through.
    const char * LastGood = Handle.FileName; // Last known valid directory path.
    while( *FileIt )
    {
	  LastGood = FileIt;
	  while( *FileIt && *FileIt != '\\' ) FileIt++;
	  if( *FileIt ) FileIt++; // We don't actually want the '\\' character itself.
    };
    // Pass on the file information if the file name matches our search criteria.
    if( this->WildCmp( Search.c_str(), LastGood, Case ) ) this->SearchCallback( Handle.FileName, Handle.FileAttrib );
  };
  return true; // Assume that everything ran successfully.
}

What is your opinion on my commenting? Which category do I fall into, from your perspective.

Attached Files

•  xfs_1_3_1.zip   15.05K   18 downloads

[+Majesticmerc]

You definitely fall into the "comment a lot" category, but as long as your keep your comments up to date, it's not necessarily a bad thing. It can prove problematic if your comments get out of sync with the actual code though.

Personally I think I sit somewhere in the middle. I believe that your comments should (for the mostpart) explain the "Why" in the code (i.e. why does this code do what it does) and let the code itself say WHAT it does. At the same time though, I think that people who insist on not commenting at all (the "all code should be self-documenting") are dangerous and make life hard for others.

For my code style, I'll generally do the following:

• Comments at the top of the class, explaining it's purpose.
  

  /**
   * Unicode string class. Holds a string of unicode characters and provides methods for manipulating the string and conversion utilities.
   */
  class UnicodeString

  
  
• Comments at the top of functions/methods explaining the method, the parameters, the return values, the exceptions returned, and any miscellaneous information relevant to users or maintainers.
  

  class AsciiString
  {
    /**
     *  Get a ROT-13 encrypted version of this string.
     *
     *  Parameters:
     *    iterations         The number of times to encrypt the string. Should be an odd number.
     *
     *  Return Value:
     *    The ROT-13 encrypted string. If the string is empty, an empty string will also be returned.
     *
     *  Exceptions:
     *    ArgumentException  The number of iterations provided was even
     *
     *  Remarks:
     *    - This method is useless. Seriously.
     */
    String GetRot13(int iterations = 1)
    {
      ..
    }
  }
  

  
  
• TODO and HACK comments. In all my IDEs these are recognised as keywords in comments, and can be useful to refer to at a later date
  

  int SomeMethod()
  {
    // TODO: Complete this method.
    throw new NotImplementedException("This method is not currently available in the beta version of the API.");
  }
  
  PoorlyImplementedInteger GetSum(PoorlyImplementedInteger values[])
  {
    // HACK: The PoorlyImplementedInteger class does not provide operators to do basic addition, but it does provide comparison
    //       and increment so we can use a loop to do the addition. As soon as addition is implemented update this method to
    //       reflect.
    ...
  }
  

  
  
• Explain the "why".
  

  string ParseMenuFromTelnetOutput(string commandString)
  {
    // Since telnet is simply just a character stream, we search for the menu input indicator ("$>") and then wait a little
    // longer in case more output is sent.
    ...
  }
  

Those examples are fairly generic, I'll often use documentation-generator syntax when applicable (in C++ I use Doxygen comments, in C# I use intellisense XML, in ruby I use rdoc, etc). I certainly try to avoid commenting every line, since it can simply end up patronising the reader, and confuse them too if the comments get out of sync with the code. Example:

// If index is zero, return false. <--- NO **** SHERLOCK!
if (0 == i) return false;

EDIT: Accidentally a code tag.

[xorangekiller]

Maybe in my example function above my commenting is a bit extreme. I don't normally comment close to every line. (Actually, I didn't realize that function was like that until you pointed it out. There used to be more content there until I abstracted it into a directory iteration class.) I do, however, normally comment every interface (class, struct, function, etc.) and every variable I create. The other comments are optional: they're only there if I decided I need them. My older code tends to have more comments inside function than my newer code does though. Overall, the source I attached to my first post is representative of my commenting and style.

[xorangekiller]

Majesticmerc, on 02 September 2012 - 20:24, said:

Personally I think I sit somewhere in the middle. I believe that your comments should (for the mostpart) explain the "Why" in the code (i.e. why does this code do what it does) and let the code itself say WHAT it does. At the same time though, I think that people who insist on not commenting at all (the "all code should be self-documenting") are dangerous and make life hard for others.

I have another example that may fit what you are talking about, albeit a bit on the extreme side. I created the rather large comment block in this function because I kept confusing myself as to what exactly I need to cleanup in this function and what was taken care of by the thread it spawns. The system as a whole that this code is part of is rather complex. This may be the single largest comment block I have ever written inside of a function.

/*
Load the specified plugin.

Remarks:
	WARNING: This function has high overhead!

Arguments:
	File [in]	   Name and path of the plugin to load.

Return Value:
	If the plugin was loaded the identification number of the thread will be returned.
	If the plugin path is invalid or the plugin has already been loaded 0 will be returned.
*/
unsigned long PluginManager::LoadPlugin( const char * File )
{
  if( File == NULL ) return 0;
  if( this->IsPluginLoaded( File ) == true ) return 0;
  this->Plugins.push_back( PluginThread() );
  this->Plugins.back().File = File;
  this->Plugins.back().Provider = new EVPlugin( this->ProviderBase, 0, this->ED, this->Tabs );
  this->Plugins.back().Provider->InfoMsg() << "Loading plugin " << File << std::endl;
  // Before we actually create the thread, here is a short analysis of how the members of this->Plugins.back() are used.
  //
  // File	   File name and path of the plugin to load.
  //			This is also used by us as a sentinel to tell when the thread is finished initializing.
  //			Since the file name is no longer needed after the plugin is loaded (and it is not guaranteed
  //			to be initialized after this function exits), NULL is used as the sentinel value.
  //
  // Provider   The plugin service provider handle that allows the plugin to interface with Alekto.
  //			It must be initialized in this function because only PluginManager can access the base.
  //			However, it does not need to be deleted anywhere in this class.
  //			Since it is used by the plugin thread exclusively, it will be deleted before the thread exits.
  //
  // Plugin	 The loaded plugin's handle.
  //			It is both created by and destroyed by the thread.
  //
  // Handle	 The handle of the plugin's thread.
  //			This handle must be assigned in this function when the thread is created.
  //			However, since it is no longer valid after the thread exits, the thread is responsible for nullifying it.
  //			This nullification is not done until the thread is ready to exit.
  //			Therefore, it may be safely used as a sentinel for thread exit.
  this->Plugins.back().Handle = CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE) &PluginThreadProc, reinterpret_cast< void * >( &this->Plugins.back() ), 0, NULL );
  while( this->Plugins.back().File != NULL ) Sleep( 1000 ); // Wait until the thread is done loading the plugin.
  Sleep( 500 ); // Wait for another half-second, just to be sure the thread has had sufficient time to cleanup.
  if( this->Plugins.back().Handle == NULL )
  {
	this->Plugins.pop_back();
	return 0;
  };
  return EVPlugin::EncodeID( this->Plugins.back().Handle );
}

[+Majesticmerc]

xorangekiller, on 02 September 2012 - 23:03, said:

I have another example that may fit what you are talking about, albeit a bit on the extreme side. I created the rather large comment block in this function because I kept confusing myself as to what exactly I need to cleanup in this function and what was taken care of by the thread it spawns. The system as a whole that this code is part of is rather complex. This may be the single largest comment block I have ever written inside of a function.

FWIW, when I said "your" in that quote I was speaking generalities, it wasn't intended as a commentary on your commenting style

[shockz]

People that say they don't comment because it's self evident are usually the ones who don't know what they're really doing and cause a massive headache for others once they are fired/leave and we're left with a jumbled mess of spaghetti code.

[Kami-]

Well in .net I use the built-in XML style commenting system - a quick example:

/// <summary>
/// An example method to show 'Commenting Style'.
/// Performs no real functionality.
/// </summary>
/// <param name="FirstName">String - Input Persons Forename</param>
/// <param name="LastName">String - Input Persons Surname</param>
/// <param name="ID">Short Integer - Input Persons ID Number</param>
public void Example(string FirstName, string LastName, Int16 ID)
{
    // Blah Blah Code Here
}

[firey]

I comment more than I probably should, my code is clean and concise, but I like to outline my code in pseudo code before I write it, then I write the code to match the comments. So you could either read the comments to know what's happening, or read the code.

[primexx]

I don't comment because I enjoy the challenge of deciphering what I wrote a year ago.

(just kidding it's actually because i don't code for a living and don't ever write anything all that complex)

[+SharpGreen]

I don't put comments in my code unless it's code I borrowed from some open source app, or something I got from a StackOverflow question. Then I will put what app it came from or the link to the SO question.

I also don't code for a living, nor do I do anything that is overly complex enough to actually require comments for someone to figure out what it does. I try to use function and variable names that would make it obvious to a reader what a particular block of code does. Same for class names.

[_kane81]

IMO I think you put in too many comments in. Code in my option should be simple and mostly self documenting. Keep It Simple and clean. Too much documentation can make it difficult to read actual code.

as some one said

I try to use function and variable names that would make it obvious to a reader what a particular block of code does. Same for class names.

Quote

return true; // Assume that everything ran successfully.

do you need to add that comment in ? ESPECIALLY when you have documented what it returns in the method header....
if you used a variable like 'hasRunSuccessfully' and then have 'return hasRunSuccessfully' it is essentially self documenting - no need for comments.

Quote

//
// Check for trouble.
//

check for 'trouble' not the best label - but programmers dont have to use the best labels, no need for the extra // just put in a blank line.

Quote

this->FoundFiles.clear(); // Clear our found files vector.

I would label this as FoundFileList or FoundFilesVector or FoundFileCollection and then if you have FoundFilesVector.Clear() it is pretty self evident what you are doing.

[+Asik]

IMO it boils down to adding relevant information where it's likely to help. There's no universal rule to determine what information is relevant and when it's likely to help. Rules like comment every function, or don't comment x or y, don't work. I am particularly vexed by any mandatory ASCII artwork like drawing boxes and lining up stars in front of every function. Complete waste of my time.

I think a combination of a clean, self-documenting coding style and unit testing goes a long way towards providing good understanding and confidence about a code base; unlike comments they can't lie and they can't go out of date unnoticed.

[Xilo]

I don't...

[virtorio]

I comment only where something is not clear and where the IDE makes use of them.

Our team follows the principals of self-explaniatory code. It's must easier to read a small function that does one thing with a set of self explanatory actions than a large function loaded with comments.

We'd probably write something closer to this for the example provided above:

unsigned long PluginManager::LoadPlugin( const char * fileName ) {

  if (!this->ShouldLoadPlugin( fileName ))
    return 0;

  PluginThread* pluginThread = this->CreatePluginThread( fileName );

  this->LoadPluginAndWait( pluginThread );

  return this->GetPluginResult( pluginThread );

}

[+Asik]

By the way, the comments are the least of your worries about the last function you posted. In order of importance:

1)

  while( this->Plugins.back().File != NULL ) Sleep( 1000 ); // Wait until the thread is done loading the plugin.
  Sleep( 500 ); // Wait for another half-second, just to be sure the thread has had sufficient time to cleanup.

Waiting half a second doesn't guarantee any particular thread will run at all. This might come up 1 time out of 10000 in particular conditions which you might not even be able to reproduce. You should be using a proper thread synchronization mechanism such as a monitor here.

2)

  this->Plugins.back().Provider = new EVPlugin( this->ProviderBase, 0, this->ED, this->Tabs );

Any "new" operator in methods is a red flag. Exceptions can be thrown anywhere (including by the "new" operator itself) and prevent any cleanup code from executing, leading to memory leaks. Plus since there's no corresponding "delete" within the method, the cleanup responsibility is apparently held by some different code, which makes your memory management difficult to reason about (and probably wrong). The only way to guarantee proper cleanup in C++ is RAII, use it.

3) Some stylistic issues:
- Inconsistent use of C/C++-style casts:

CreateThread( NULL, 0, (LPTHREAD_START_ROUTINE) &PluginThreadProc, reinterpret_cast< void * >( &this->Plugins.back() ), 0, NULL );

- Using "this->" in front of every member access is just syntactic noise.

- If you're using a recent C++ compiler (VS2010 and above), use nullptr instead of NULL. nullptr is a pointer type, NULL is an integral constant. Consider:

void foo(int*);
void foo (int);

void bar() {
  foo (NULL);  // Calls 'foo(int)'
}