Code style guidelines and cleanup/work

[oln]

Some of the styling can be done by a good refactoring tool, which can save you for a lot of work.

[StefanP.MUC]

Can you recommend some good tools?

[oln]

Seems like eclipse is able to do it (source -> format)

[oln]

Can't seem to make it do it automatically for all files though.
If we decide on a style setting, we can start going through and fixing this in all the files.
It is possible to export the style settings, so if we share one we can take one part of the source each.

[oln]

Does this look okay?

{l Code}: {l Select All Code}

/*
 * A sample source file for the code formatter preview
 */
#include <math.h>

class Point
{
    public:
        Point(double xc, double yc) :
            x(xc), y(yc)
        {
        }
        double distance(const Point& other) const;

        double x;
        double y;
};

double Point::distance(const Point& other) const
{
    double dx = x - other.x;
    double dy = y - other.y;
    return sqrt(dx * dx + dy * dy);
}

[StefanP.MUC]

Yes, this looks quite good.

[oln]

Going to go through the source files and format them then.

[oln]

Okay, I've gone through all the source files, ran the formatting tool on them, and pushed them to git.
For everyone that is editing code, make sure to use spaces and not tabs.

[StefanP.MUC]

I just pushed another commit, changing more postfix++ to ++prefix. But I noticed some operations where I was not sure if they are intended:
GameMap.cpp, line 2533

{l Code}: {l Select All Code}

tempIterator = currentThreadReferenceCount++;


In this code "tempIterator" will get the old value of "currentThreadReferenceCount" (which is raised by one after the assigment to the left).

The same thing happens on the following files. But there we have only string outputs (Maybe wrong outputs, but not affecting the game logic).
MapLight.cpp, line 16
MissileObject.cpp, line 27
RoomObject.cpp, line14

[oln]

Working on cleaning up includes/headers and use forward declarations where possible, so please avoid doing to much with the code until I'm finished with it, as it can easily create problematic merges.

[oln]

Noticed a bunch of things while skimming trough that probably should be changed:

• Casting of const chars to strings:
  

  {l Code}: {l Select All Code}

      return (string) "Protect the creature named " + creatureName + ".";


  
  this seems rather odd, should use the string constructor instead
  

  {l Code}: {l Select All Code}

      return std::string("Protect the creature named ") + creatureName + ".";


• Not using const references where they should be used, and instead copying objects. Saw this mainly with strings:
  

  {l Code}: {l Select All Code}

  Goal::Goal(std::string nName, std::string nArguments)
{
    name = nName;
    arguments = nArguments;
}

  
  No need to copy the strings twice here:
  

  {l Code}: {l Select All Code}

  Goal::Goal(const std::string& nName, const std::string& nArguments)
{
    name = nName;
    arguments = nArguments;
}

  
  
  (Though, BE CAREFUL if changing classes that derive from other classes as function overloading may cause the wrong function to be called if one of the classes has a function with refs and on has the function without)
• using statements in headers
  This pollutes the namespace, should be avoided. Fixed quite a bit of it, but still some instances left.
• Public variables
  There seem to be a lot of inconsistency with this, some places public vars are used, even though there are getters and/or setters.
• A lot of inclusions in headers
  What I am working on at the moment, should use forward declarations and put includes in the cpp files when possible.
• C-style casts
  Should be avoided, use static_cast, (or reinterpret_cast where needed).
• unsigned int - should be enough to just write unsigned (or is there any reason not to that I don't know of?)

[StefanP.MUC]

After your commits are pushed, I'll see what I can do about the variables and getters/setters. This shouldn't be to hard to unify and use getters/setters consistently.

I stopped working on the for loop optimizations currently (moving calls out of the condition), because for example the creature routine is a 1000+ line block with like 20-30 nested loops. It get's really nasty to keep the overview and not break something...

[Bodsda]

• unsigned int - should be enough to just write unsigned (or is there any reason not to that I don't know of?)

{l Code}: {l Select All Code}

unsigned int foo;
unsigned foo;


Those statements are equivalent. However, for clarity, it may be worth using the former.

Bodsda

[svenskmand]

I stopped working on the for loop optimizations currently (moving calls out of the condition), because for example the creature routine is a 1000+ line block with like 20-30 nested loops. It get's really nasty to keep the overview and not break something...

Heh that screams so much for refactoring When I write Java I never have methods that are more than a couple of screens of code, when it is worst Of course they can be big at some point, but when I find myself scrolling the screen to much I know it is time to refactor the code

But 20-30 nested for-loops that is crazy, wonder how Andrew can keep track of what is going on there, let alone the trouble of having to deal with 20-30 different variable names for iteration in each loop :S

[oln]

Pushed a huge commit with a lot of header and namespace cleanups.

[StefanP.MUC]

"unsigned" is exactly the same as "unsigned int". But there's also "unsigned char", for example. So just out of consistency and clarity I vote for the version with int:

{l Code}: {l Select All Code}

unsigned int foo


Sorry, I was a bit unclear, the 30 for loops are not nested 30 levels. There are this big amount of loops, and some of them are nested up some levels:

{l Code}: {l Select All Code}

for()
{
    for()
    {
        for(){}
    }
}
for(){}
for()
{
    for(){}
}
//this is going on that way...


[oln]

Any reason why you want to only use C-style comments?

[StefanP.MUC]

You mean this ones: /* */ ?
Most comments are multiline anyway (escpecially if we use doxygen they get long), they are easier to extend, because you don't need to add the // by hand on every line. And the rest can be this way to, just out of consistency.

And a personal reason on my site: I don't like the double slashes (//) in code, it doesn't look nice when there are hundreds of lines starting with "//", IMHO. They are good on short single lines in example code, yes, but in bigger projects I prefer consistency.

If there's a reason to use mixed comments or only the double slashes, we can drop this point from the guidelines, of course, or make it less strict.

[Bodsda]

You mean this ones: /* */ ?
Most comments are multiline anyway (escpecially if we use doxygen they get long), they are easier to extend, because you don't need to add the // by hand on every line. And the rest can be this way to, just out of consistency.

And a personal reason on my site: I don't like the double slashes (//) in code, it doesn't look nice when there are hundreds of lines starting with "//", IMHO. They are good on short single lines in example code, yes, but in bigger projects I prefer consistency.

If there's a reason to use mixed comments or only the double slashes, we can drop this point from the guidelines, of course, or make it less strict.

I don't see a reason to limit commenting styles. They each have there purposes. I would not expect to see // on multiple consecutive lines unless side-by-side explanation is needed like say for a group of complex mathmatical calculations

Good

{l Code}: {l Select All Code}

someReallyComplicatedEquation_1 // this produced theUniverse
someReallyComplicatedEquation_2 // this allowed theUniverse to expand
someReallyComplicatedEquation_2 // This set the return value of theUniverse to 42


Good

{l Code}: {l Select All Code}

/* This next section of code contains someReallyComplicatedEquation_s
that define theUnivers, its behaviour and its return value
DO NOT CHANGE!! */


Bad

{l Code}: {l Select All Code}

//  This next section of code contains someReallyComplicatedEquation_s
// that define, theUniverse, its behaviour and its return value
// DO NOT CHANGE!!


Bad

{l Code}: {l Select All Code}

someReallyComplicatedEquation_1 /* this produced theUniverse */
someReallyComplicatedEquation_2 /* this allowed theUniverse to expand */
someReallyComplicatedEquation_2 /* This set the return value of theUniverse to 42 */


[svenskmand]

I agree wit Bodsda on this one, both styles have their justification.

[oln]

+1

[StefanP.MUC]

Ok, I dropped the comment part of the guidelines.

[svenskmand]

Ok, I dropped the comment part of the guidelines.

Do not drop it, but add Bodsda's guidelines, they seem pretty sane.

[oln]

Regarding member variables. Should we use a pre-/postfix, or not?

[StefanP.MUC]

I also thought about this and came to the conclusion to not use any *fixes on member variables:
- *fixes lower readability
- in general, coders should avoid giving the same name to different things
- for not-avoidable cases, C++ has "this->"

Example:

{l Code}: {l Select All Code}

//BAD
int member;
void setMember(int member)
{
    this->member = member;
}

//GOOD
int member;
void setMember(int newMember)
{
    member = newMember;
}