PROJECT: Coding conventions and standards

[Keygen]

It has been discussed before but not adequately, nor final decissions has been made on the issue in my opinion.
What coding style should we adopt, if any? Is it necessary? Should we follow some standards?

I beleive in some cases it could be critical.
Generally it would help a bit if we would put some fundamental rules.

a) ANSI compliant code. We should code exclusively under the ANSI standard unless absolutely necessary to go with Microsoft Visual C++ specific coding.

b) Comments. We can go with fromafar's style, perhaps add some additional information in the header of each file altered. I recommend we add comments at the start and the end of the new code we enter to mark the start and end of it, plus the name of the coder who added the code, and a small description if needed. Whether \\ or /* */ should be used or a combination is under discussion also.

c) Naming style. What we need here is a descriptive name for each variable, constant, object etc. Secondary styling like camel-notation, hungarian-notation, underscore character usage etc. can be discussed but I don't think they are important and each coder could co with his style, perhaps with a common agreement on the uppercase and lowercase letters.

d) Whitespace. How much whitespace should be used to sort the code and make it more readable? How much do we indent?

e) Braces. Can the way braces are put cause problems?

f) Long lines. Should we keep long lines or break them down into more?

Also other areas such as access, class definitions, include files, assertions, libraries, etc. can be discussed here whether it is early or not and will be of more use when we start to make extensive changes and additions to the code.

[Martin Gühmann]

Re: PROJECT: Coding conventions and standards

Originally posted by Keygen
b) Comments. We can go with fromafar's style, perhaps add some additional information in the header of each file altered. I recommend we add comments at the start and the end of the new code we enter to mark the start and end of it, plus the name of the coder who added the code, and a small description if needed. Whether \\ or /* */ should be used or a combination is under discussion also.

Actual it is not necessary to add comments at the end of each new piece of code as the #endif clearly states it. Maybe we could put a //ACTIVISION_ORIGINAL behind it, but actual it only does make sense if the new piece of code is very long, so that you can't see the start and end of the new code on one glance.

-Martin

[ctplinuxfan]

Re: PROJECT: Coding conventions and standards

Hi,

Originally posted by Keygen
a) ANSI compliant code. We should code exclusively under the ANSI standard unless absolutely necessary to go with Microsoft Visual C++ specific coding.

Yes, this would be wonderful (especially for porting)... ;-)

Originally posted by Keygen
b) Comments. We can go with fromafar's style, perhaps add some additional information in the header of each file altered. I recommend we add comments at the start and the end of the new code we enter to mark the start and end of it, plus the name of the coder who added the code, and a small description if needed. Whether \\ or /* */ should be used or a combination is under discussion also.

I think comments should be used to document how non-trivial code works, but not be used for each code change. Guess something like this:

// Coder: blafasel
// Fixed case of header
#include "C3Something.h"
// EOF: Coder blafasel
// Coder: bar
// My implementation
#include "IBar.h"
// EOF: bar

// Coder: bar 
// Added Bar::foo()
void
Bar::foo()
{
  // ... (code by bar)

  // Coder blafasel
  // fixed error handling of code

  // ... ( some code by bar)

  // EOF Coder blafasel
}
// EOF Coder Bar::foo()

This is a simple example. What about nested changes (by different coders), etc? You don't see the code after a while, but mainly a bunch of comments with some code inbetween.
If that information is needed, it would be better if a central version control system like cvs would be officially used, so each coder could enter that information on commit. When reviewing code changes, you could have a look at the logs and do a cvs diff between the wanted revisions. As each coder had his own login, the user name in the log would be the "who changed" info.
We could stick files to a simple header containing a contributed line where everybody wanting to do so may add his credits.

Originally posted by Keygen
c) Naming style. What we need here is a descriptive name for each variable, constant, object etc. Secondary styling like camel-notation, hungarian-notation, underscore character usage etc. can be discussed but I don't think they are important and each coder could co with his style, perhaps with a common agreement on the uppercase and lowercase letters.

I think variables should be capsulated and therefore they are hidden for the holder of an objects instance. I'd say for access there should be getters/setters (get/set prefix), with capitaled short descriptive what the getter/setter returns/updates.

Originally posted by Keygen
d) Whitespace. How much whitespace should be used to sort the code and make it more readable? How much do we indent?

I'd say the best would be space identation of say 2 or 3 spaces per indent level. Space indentation gives a similar look regardless what editor you choose, thus everybody sees the same structured code on his screen. Any editor i know is configurable using space indentation instead of inserting tabs.

One thing to add: Line delimiters should only be newline (\n), and not carriage return (\r) additionally. I.e. before posting, use dos2unix on that file.

Originally posted by Keygen
e) Braces. Can the way braces are put cause problems?

I use a mixed style for that. In short code, they all go in one line.
Within long expressions, i also put the closing ")" indented under the corresponding "(". {} pairs always have same indentation. It's just for readability, you could put them where you want...

Originally posted by Keygen
Also other areas such as access, class definitions, include files, assertions, libraries, etc. can be discussed here whether it is early or not and will be of more use when we start to make extensive changes and additions to the code.

I often use .hpp extension for include files, so *.[ch]pp are c++ source files in shell and for tools like find.
There are some recommendations on
C++ Recommendations
Mozilla C++-Portability guide

Oh, and prefer the last one over the first one.


Ciao
Holger

[Solver]

A CVS will indeed be needed so that code files changed by a dozen coders don't start to have 100 kb worth of comments.

The hard part to agree upon might be bracing style. I have a habit of putting the opening { on the same line as the if or for statement. Also, I will always surround the conditional code (after an if statement) with a pair of braces, even if there's only one line of code, where the braces can be skipped.

[ctplinuxfan]

Hi Solver,

Originally posted by Solver
The hard part to agree upon might be bracing style. I have a habit of putting the opening { on the same line as the if or for statement. Also, I will always surround the conditional code (after an if statement) with a pair of braces, even if there's only one line of code, where the braces can be skipped.

Well, there indeed many bracing styles, and i prefer yours.

The style most uncomfortable for me is the gnuish style (many control statements and you have to scroll, scroll, scroll and you don't know what was in the beginning, so you scroll, scroll and scroll...).

Ciao
Holger

[vovan]

So, the issue of documenting the code has been brought up, and I think it is fairly important to have the code documented well (and I mean more of documenting the API of different modules even than simply explaining tricky pieces of code inline, though that would be nice too).

I suggest using some commenting style that is understood by doxygen, because it is the best tool to generate HTML documentation for the code that I know of for C++. That being said, it does take a loooong time to generate all docs for the CtP2 project. I just ran doxygen yesterday, and it took over 20 minutes, I think, and the resulting documentation is almost 300MB, and remember that is without *any* comments (I ran the doc generator on the original sources).

So, I suggest using one of doxygen's commenting styles: either javadoc style:

/**
 * Description of func.
 *
 * @param p Description of param.
 * @returns Description of return value.
 * @author Who last messed with it.
 * @date When was it last messed with.
 */
int func(char* p) { ... }

I personally, prefer this documentation style, but that's just me. doxygen also understands:

///
/// Description goes here
///
int func(char* p) { ... }

/////////////////////////////////////////////////
/// Description goes here
/////////////////////////////////////////////////
int func(char* p) { ... }

//!
//! Description goes here
//!
int func(char* p) { ... }

And others. So I'd say - use whichever one you want - as long as doxygen can understand it.

More on doxygen commenting standards here:

http://www.stack.nl/~dimitri/doxygen/docblocks.html

-----

Now, there is currently no particularly organized documentation effort, and I don't know that it is terribly needed - a lot of code will probably be never messed with any way - though I think it would be a nice thing to do, in case someone wanted to do some major changes.

So, what I suggest is that when people fix bugs - document stuff that you mess with. What I mean is: you figure out what's going on any way - share that knowledge. Suppose you change function foo in class Bar in file Bar.cpp. Then document function foo. Say function foo does such and such, and put in a revision history saying on date such and such, I, such and such, made changes this and that, so that such and such works properly now. Maybe if class Bar isn't that big, even document the whole thing.

That way, while we are not exactly spending effort exclusively documenting the code, documentation is still being done. It is better than nothing.

Just a suggestion.

[J Bytheway]

Originally posted by vovan
So, I suggest using one of doxygen's commenting styles: either javadoc style:

/**
 * Description of func.
 *
 * @param p Description of param.
 * @returns Description of return value.
 * @author Who last messed with it.
 * @date When was it last messed with.
 */
int func(char* p) { ... }

I personally, prefer this documentation style, but that's just me.

I dislike this style, because it prevents commenting out large blocks of code using /* ... */, but perhaps that's just me .

Personally, I'd probably use ///, since that's what C# needs for its documentation generation.

[Martin Gühmann]

Originally posted by J Bytheway
I dislike this style, because it prevents commenting out large blocks of code using /* ... */, but perhaps that's just me .

Personally, I'd probably use ///, since that's what C# needs for its documentation generation.

Well in that case we should use ///, it is also less work to convert the existing comments, As we dont have to bother with the existing lines:

//-------------------------

I would like to keep these lines, as they make the code more readable, but of course they shouldn't be part of the documentation generated by Doxygen.

-Martin

[vovan]

Originally posted by J Bytheway
I dislike this style, because it prevents commenting out large blocks of code using /* ... */, but perhaps that's just me.

Preprocessor is your friend.

Do

#if 0
...large chunk of code...
#endif

instead of

/*
...large chunk of code...
*/

[J Bytheway]

Originally posted by vovan
Preprocessor is your friend.

Do

#if 0
...large chunk of code...
#endif

I don't like that either because it doesn't get syntax-highlighted, and is easy to miss for others browsing the code .

[Lui2]

Asserts

What is the opinion or standard for handling assert messages that will be raised (also) in valid situations?

IMO this should changed in cases wheer it is possible, so that only asserts will be declared that are completely valid. Because that leads to a debugging where only than a assert message pops up, when there is really a problem.

And I added the example, that brings me to this question. The comment was addded by me after searching for the reason I got the assert message.

By the way: If anyone has some suggestions what to do, to change this assert or the dependant code so that only in real wrong situations a assert message pops up, i'm really interested to hear.

If someone wants a savegame, that causes the assert by finishing the current turn, let me know to add one.

[J Bytheway]

It seems to me what should be happening is that contact should be initiated by the combat, before war is declared... Any circumstances leading to a declaration of war should certainly cause contact.

[Fromafar]

In this particular case, I would suggest weakening or removing the Assert, because this is a valid game situation, and the game does handle the situation well. If the slaver is invisible to the victim, no contact is established. The victim may be enraged about the captured citizens and would like to declare war, but can't, so actually doesn't.

You may still want to Assert the validity of player_ptr and foreigner_ptr, but the current Assert is not helping in finding bugs. It is merely irritating. Fortunately, it does not really occur many times, and all you have to do is press Ignore.

[J Bytheway]

I thought this was the situation of a slaver attack, not a slave raid. In this case, the victim can see the slaver (when it attacks).

[Lui2]

We seem to agree, that a assert should only be set, if the condition of the assert is always true, as long the program works correct.

This leads to the need to change something about the handling of the assert in this special case with the slave driver.

More questions raise for me now with the ctp2 code:

1) What about unused functions, methods or classes? Shall they commented out or removed from the actual further developed code to reduce complexity?

2) I'm not sure about the ACTIVISION_ORIGINAL thing, that will be used now, to indicate the changes. As more and more changes are made to the code, this clutters and blows up the source and it is harder to look through the code and understand and/or change him.

It would be much better to have a kind of routine to make versions of the different source files. So you have always the prior versions of the file to see what's changed in time using e.g. WinDiff or another tool. And your actual source is clean and only includes actual needed stuff.

To compile and build the orignial you can use an archiv of the sourcefiles.