Please Review My Coding Style

[zb0S216C]

I've seen users of this forum posting a C++ source file containing their coding style. So I thought I would have a go :)

Rating System
1 asterisk( * ): Poor
5 asterisk( s ): Excellent

Note that you aren't rating the functionality of the code, only the style.

Example Source File

// Include: C-Standard I/O.
// Usage: For C-Standard I/O stream abilities.
#include <cstdio>

// Include: C-Standard Library.
// Usage: Includes the NULL keyword.
#include <cstdlib>

// Class Name: ExampleClass
// Usage: Serves as an example class for this sample source file.
class ExampleClass
{
    public:

        // Constructor Version: Default
        explicit ExampleClass( void )
        {
            // Build Member: Value
            // Initialization: Using a default value of zero.
            Value = new int( 0 );
        }

        // Constructor Version: Copy
        // Parameter 1: A reference to an instance of ExampleClass.
        //              This will be used as the source of initialization.
        ExampleClass( const ExampleClass &Class )
        {
            // Build Member: Value
            // Initialization: Using the value of: Class.Value.
            Value = new int( *Class.Value );
        }
 
        // Constructor Version: Initializer
        // Parameter 1: The initializing value of Value.
        explicit ExampleClass( const int InitValue = 0 )
        {
            // Build Member: Value
            // Initialization: Using the value of: InitValue.
            Value = new int( InitValue );
        }

        // Destructor
        // Usage: Destroy any members that allocate memory.
        ~ExampleClass( void )
        {
            // Destroy Member: Value
            delete Value;

            // Action: Set Value to NULL to prevent an invalid memory address.
            Value = NULL;
        }

        // Member Type: Method
        // Usage: Multiplies Value by the given value( as an argument ).
        // Parameter 1: The multiplication value. Example: Value * MultiplyBy
        int Multiply( const int MultiplyBy = 0 )
        {
            // Return: The computation result of: Value * MultiplyBy
            return( *Value * MultiplyBy );
        }

        // Member Type: Variable
        // Usage: Contains an integer value that will be multipled by Multiply( ).
        int *Value;
};

// Main entry-point.
int main( )
{
    // Instance of: ExampleClass
    // Usage: Serves as the source for the copy constructor of Destination.
    ExampleClass Source( 10 );

    // Instance of: ExampleClass
    // Usage: Uses the copy constructor to initialize it's member( s ).
    ExampleClass Destination( Source );

    // Print: The value of: Destination.Value.
    printf( "The value Destination.Class is holding is: %i.\n", *Destination.Value );

    // Return: zero
    return 0;
}

[cainen172]

****
it is good and you can keep track good with all of the notes. but some of them just seem un-needed

[zb0S216C]

Thanks for the reply, cainen172.

but some of them just seem un-needed

Better safe than sorry :)

[subarde]

**** it's awesome but some comments are just a waste of time like:

// Main entry-point.
// Return: zero 

[hamsterman]

I'm not really sure what exactly "coding style" is. I have a feeling it's mostly about whether you white "{" on the same line as "if" or not. So I'll ignore it. I'll review something instead..

I know this is just a random example, but with your code operator = causes a memory leak.
What is that "explicit" doing with a default constructor? I believe it has no effect there.
Why set Value to 0 in destructor? In what scenario could (should) you be able to access Value after calling the destructor? Explicit destruction?
When writing C++ use C++ standard libraries. It's just prettier.
Why NULL instead of 0 and "( void )" ? Again, this is not C.
You're abusing comments..
"return" doesn't need (). Though I suppose you could find that cleaner..

*** or **** maybe. Though I really don't know how * or ***** code could look. Also there's just too little of it to see serious problems.

[Moschops]

// Instance of: ExampleClass

That sort of thing doesn't add to the readability of the code, and adds unnecessary clutter. I can tell that it's an instance of the ExampleClass by looking at the code itself:

ExampleClass Destination( Source );

Here's another one:
ExampleClass( const ExampleClass &Class )

I can tell by loking at it that the first parameter is a reference to a const Exampleclass. The comment next to it,
// Parameter 1: A reference to an instance of ExampleClass.
doesn't tell me anything I didn't already know from reading the code and adds unnecessary clutter.

Comments should add to my understanding where it's plausible that I can't understand purely by looking at the code. If the comment simply repeats the code, it is unnecessary, adds clutter and is also dangerous; comments rot over time, by which I mean that it's very common for people to change code and not be as scrupulous as they should be in updating the comments, so you end up with comments that are actually wrong.

Write as much commentary as is needed, and not a single character more.

[simeonz]

First, in my short career, I have seen code that would repel a troll. That being said, and simply commenting randomly (and subjectively), here it goes:

-- Don't comment on all semantics of the code inside the code. It clusters it too much. When you do, make the comments short one liners (incomplete sentences), and try to put them to the side of the code. Primarily, IMO branching conditions deserve commenting.

-- Specific technical comments can be useful and in fact are sometimes mandatory. For example, if you miss default statement in some switch, explain why. But not stuff that is conventional. Like, don't comment why you don't free the storage held by auto_ptr (since the latter does that automatically).

-- Don't explain the coding style. There are documents for the coding style of the entire project. If you have some aspects of your coding style that you want to communicate to the rest of the team and future maintainers, then find another form to do it. At least do it in the documentation header of the source file if you have to, but not inline. You see, once the coding style is fixed, comments that assert its purpose on every line contribute very little, but boy do they cluster the code.

-- Don't educate using comments. You want to force some peer into learning something - volunteer for their next code review :)

-- Use separate documentation for algorithms, theoretical results, etc. Managing documentation is hard to impossible. But still, only short reference documentation should be part of the code. Not only to save space in the source file, but also because explaining an algorithm without block diagram (flow chart) just won't fly. Use of visual aids is important and inline doc is not good at that.

-- Investigate smart pointers. Particularly auto_ptr, boost::shared_ptr. You wont have to free memory in your destructor, and your code will be exception safe. That being said, there is not enough functionality here to demand their use.

-- Of course, header files and what have you are needed. But this is a forum post, so it's ok ;)

Regards

P.S. : If you see some of the messes I did produce occasionally, we would have another talk lol

[ne555]

It does not compile

explicit ExampleClass( void );
explicit ExampleClass( const int InitValue = 0 );

ExampleClass foo; //error: call of overload ExampleClass() is ambiguous 

I don't like this default parameter (as default not the number itself) int Multiply( const int MultiplyBy = 0 )

It could be a good practice to separate implementation from definition. And maybe put it in its own header and source files.

Also int *Value;. why a pointer? It could be better to just use an integer.
And it is public. So the client could easily invalidate it.

[zb0S216C]

It does not compile

Mine compiles perfectly fine, no errors, no warnings( even with all compiler flags enabled ).

It seems that people aren't reading the description at the top of the post. It specifies that you review the coding style( Comments and formatting ), not the code( A primary example given by ne555:

int *Value;. why a pointer?

).

Thank you all for your input :) It's come to my attention that more questions have been raised about the commenting of the given code. I've taken into account all the reviews and I shall use the remarks in future projects.

Thank you all :)

Edit-----------------------------------8<-----------------------------------

boost::shared_ptr

I don't use Boost. I've developed my own custom auto_ptr that I prefer to use.

[Moschops]

As an addendum to my earlier post, this comment:

// Return: The computation result of: Value * MultiplyBy

is simply plain wrong. The function does not multiply Value by MultiplyBy, but instead dereferences Value and multiplies that, but now you've got the reader confused; is the bug in the code or the comment? Maybe they don't even notice that the comment and code disagree and just believe the comment. The comment would add nothing if it were correct, and is damaging when wrong.

[zb0S216C]

The function does not multiply Value by MultiplyBy

I tested the Multiply( ) method and it returns 60( Source.Multiply( 6 ) ). If I change the return statement, the compiler complains.

[ne555]

You aren't testing the default constructor. The error is produced because you want to provide a default argument to every function.

[Moschops]

Framework wrote:

I tested the Multiply( ) method and it returns 60( Source.Multiply( 6 ) ). If I change the return statement, the compiler complains.

Your comment says you are multiplying Value. You are not multiplying Value. The comment is just plain wrong. The code is fine. The comment is wrong.

[zb0S216C]

You are not multiplying Valu

Then instead of telling me what it's not doing, tell me what it is doing.

[Moschops]

I did. Read the post timed 4:36pm. The sentence beginning

The function does not multiply Value by MultiplyBy, but instead

is what you're looking for.

[Kiana]

I don't really like it when functions are called like func( x ) instead of func(x). It seems like a lot of unnecessary whitespace. Also, there is no need to comment everything. Your code should be self-reflective.

I also think code looks sexier when it uses two spaces for tabs instead of four.

[3hM2Nwbp]

I usually comment the living hell out of my headers, but leave the source files barren (of comments) unless I do something tricky and/or arcane.

Overall ****

*Edit

I don't use Boost. I've developed my own custom auto_ptr that I prefer to use.

Still, it would be advisable to look under the hood at boost's smart pointers. There is a lot more going on than what you might think. Checked deleting and (I read somewhere, but haven't investigated it myself) thread safety. If nothing else you could adapt your implementation to incorporate some of the aspects of boost's work.

*Edit2
You might also consider trying to run your code through a service like doxygen. I'm not sure if your commenting style is supported by it or not, but (you might know) doxygen produces documentation similar to Javadoc. There are VERY few things that I like about Java, but the standardized documentation is one of them.

[jsmith]

Regarding comments: there isn't a single comment in there I find worth keeping. You should strive to make code as self-documenting as possible, and add comments in the following circumstances:

1) If it is not *painfully* obvious what a piece of code does, then document the what.
2) Document why the code does what it does.
3) If there is a subtlety about a section of code, function, or class, document it.

Everyone knows what the standard headers are for. If you want to document anything regarding them, you could write what symbol(s) you are using from them (ie, why you included it). However, be careful about doing this because it is a maintenance problem.

Everyone knows how to recognize a default constructor, copy constructor, and destructor; no need to say that's what it is. If the copy constructor does what you'd expect any copy constructor to do -- deep copy everything -- then there is no need to document it. Document only if your copy constructor does non standard (non-intuitive) things. Which hopefully it does not.

As far as coding style... everyone has their own favorite coding style regarding placement of whitespace, braces, etc, etc. There is no "right" coding style, but there are "wrong" ones. Yours is fine as is.

As far as algorithms, etc... there isn't enough substance there to comment on.

[JoR]

I can't vote, see how I'd have done it, decide how you like it (may have some minor mistakes, didn't double-check or try it):

//main.hpp

#include <cstdio>

#include "ExampleClass.hpp"

int main( ) {
	ExampleClass source( 10 );
	ExampleClass destination( source );

	printf( "The value Destination.Class is holding is: %i.\n", destination.getValue() );

	return 0;
}

//ExampleClass.hpp
#ifndef EXAMPLE_CLASS_HPP
#define EXAMPLE_CLASS_HPP

// For demonstrational purpose only.
class ExampleClass {
private:
	int value;
public:
	ExampleClass( );
	explicit ExampleClass( const int );

	int multiply( const int );
	int getValue( );
};

#endif //EXAMPLE_CLASS_HPP 

//ExampleClass.cpp

ExampleClass::ExampleClass( ) {
	value=0;
}

ExampleClass( const int initValue ) {
	value = initValue;
}

int multiply( const int factor ) {
	return value * factor;
}

int getValue() {
	return value;
}

[Zaita]

+1 JSmith

I don't use Boost. I've developed my own custom auto_ptr that I prefer to use

This to be is a big negative. Why would you not use an industry accepted standard container? What makes you think your one is better? more scalable? more stable? more tested?

There is no way I'd let one of my developers implement something like this. Re-inventing the wheel is a waste of time (= money)