Code style guidelines and cleanup/work

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 12:44

Some of the styling can be done by a good refactoring tool, which can save you for a lot of work.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 17 Mar 2011, 13:06

Can you recommend some good tools?
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 13:25

Seems like eclipse is able to do it (source -> format)
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 13:34

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.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 13:46

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);
}
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 17 Mar 2011, 14:38

Yes, this looks quite good.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 14:43

Going to go through the source files and format them then.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 14:59

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.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 17 Mar 2011, 15:36

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
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 16:49

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.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 18:17

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?)
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 17 Mar 2011, 18:44

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...
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby Bodsda » 17 Mar 2011, 18:52

oln {l Wrote}:
  • 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
User avatar
Bodsda
OD Moderator
 
Posts: 195
Joined: 18 Feb 2010, 08:19

Re: Code style guidelines and cleanup/work

Postby svenskmand » 17 Mar 2011, 19:18

StefanP.MUC {l Wrote}: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 :D

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
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 17 Mar 2011, 19:43

Pushed a huge commit with a lot of header and namespace cleanups.
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 17 Mar 2011, 19:51

"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...
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 24 Mar 2011, 13:01

Any reason why you want to only use C-style comments?
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 24 Mar 2011, 13:23

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.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby Bodsda » 24 Mar 2011, 15:44

StefanP.MUC {l Wrote}: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 */
User avatar
Bodsda
OD Moderator
 
Posts: 195
Joined: 18 Feb 2010, 08:19

Re: Code style guidelines and cleanup/work

Postby svenskmand » 24 Mar 2011, 16:04

I agree wit Bodsda on this one, both styles have their justification.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 24 Mar 2011, 16:09

+1
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 24 Mar 2011, 17:42

Ok, I dropped the comment part of the guidelines.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby svenskmand » 24 Mar 2011, 17:53

StefanP.MUC {l Wrote}:Ok, I dropped the comment part of the guidelines.

Do not drop it, but add Bodsda's guidelines, they seem pretty sane.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 27 Mar 2011, 12:22

Regarding member variables. Should we use a pre-/postfix, or not?
User avatar
oln
 
Posts: 1020
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 27 Mar 2011, 12:35

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;
}
StefanP.MUC
 

Who is online

Users browsing this forum: No registered users and 1 guest