Mittens 2018 will be in Boston this September. There are three spaces left, so check out the thread for details!

Author Topic: AGS Engine Coding Conventions - Proposal Draft 1.0  (Read 11974 times)

monkey0506

  • AGS Project Tracker Admins
  • Tasting the banhammer. Strangely, tastes like ham.
As we're moving forward with more developers starting to get into the engine code, we need to define a well structured set of conventions which everyone can follow and conform to. This will promote consistent programming from anyone contributing to the source. I will include here a proposal for the AGS Engine Coding Conventions Draft 1.0. Anyone is free to voice their opinion about this proposal, and it will be modified according to popular trend. As a point of reference I have used the Google C++ Style Guide as well as ScummVM's Code Formatting Conventions and Coding Conventions, along with some personal preference.

For the sake of readability and making it easier to update, the AGS Engine Coding Conventions are now in the wiki. This should be considered the "official" draft/proposal (unless otherwise noted).

(Regarding updating, not only can other individuals not edit my posts of course, but also this post was dangerously close to the 30,001 character limit the forums have for a single post, and things were already having to be moved around...)
« Last Edit: 10 Feb 2014, 09:08 by Snarky »
User was banned for this post.

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #1 on: 30 Jun 2012, 21:28 »
Monkey, you are a bit insane, do you know this? :) I still think you could make a separate document, like WyZ did with his Big Plan. I hope you will do that with final CC version.

monkey0506

  • AGS Project Tracker Admins
  • Tasting the banhammer. Strangely, tastes like ham.
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #2 on: 30 Jun 2012, 21:47 »
When will you people ever learn that 99% of everything I do, I do simply because it's fun to do so?

The few games I've released were a blast to make. In fact, in that Monkey Island "parody"(? if it can be called that), I actually had a function called "DoThatSpeechThingThatYouLoveToDoSoMuchB asedOnWhoTheCurrentPlayerIs" or something like that, which was only ever called from within a function simply titled "Do" (of course the source for that is sadly lost in the sands of time, moving, computer hard drive crashes, etc.).

In my OSD game, I programmed hundreds of lines of code to get semi-realistic RPG-style character battle statistics, much of which served absolutely zero purpose in-game.

Sure, I could have put this in the wiki, or made a Google Doc, or etc., etc., but where's the fun in doing that? Creating a huge thread that has to span two separate posts just to fit...now that's what I'm all about. :D

Of course, I wouldn't want to see this type of childish behavior exhibited in the AGS engine source code, which is why we need some coding conventions...oh, wait. :P
« Last Edit: 30 Jun 2012, 22:47 by monkey_05_06 »
User was banned for this post.

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #3 on: 30 Jun 2012, 23:29 »
Some thoughts on all this.
Please keep in mind, that many points in code conventions are usually stated in sake of consistency rather than better design or optimization.
For these I'll simply mention alternative arguments.

Quote
1.3 Braces
"Curly braces", { and }, should occupy a line by themselves (trailing comments are acceptable also). They should be at one level of indentation lower than the code they contain. This helps as a vertical guide (in addition to the indentation) in finding where a particular block of code starts or ends.
I used this style for a very long time, until one day I was asked by our lead dev to put opening brace on the same line as "if" and "for".
He explaned that thus more actual code lines will fit in the screen. At first that may seem a bit ridiculous, but not after you spent months studying hungreds of thousands of lines in 10+ years-old code :)
On other hand, that depends on average function lengths. If we are going to stick to writing smaller functions this may be not that important.

I did not notice if you've mentioned a rule of making each "if" and "for/do/while" contents be enclosed even if they consist of one line. This is sometimes suggested for it would be harder to make a mistake and add new line, forgetting to enclose it in brackets.

Quote
1.5 Naming Conventions
...
Local variables (inside of functions) can be named using either lower camelCase or c_style_notation, but should remain consistent throughout any single file.
Personally I like c_style_notation for function parameters and local vars more for some unexplainable reason :)
Also, if class private data is named using "lower" camelCase, using c_style_notation for locals will help to distinguish them.
There could be an interesting case though if private member's name consists of one word; in that case it would be indistinguishable from local vars. There may be options how to solve this, for example add an extra char as either prefix or postfix.

Regarding function names. There are two main options for how to form a function name, I've seen both being suggested in the web -
a) verb first, e.g. AddTableEntry();
b) noun first, e.g. TableAddEntry().
Personally I suggest this:
- for class members it should start with verb, and should not include owning class name; for example if we have class Table, member function should be AddEntry and not AddTableEntry. (Except if the function is supposed to deal with other object, like CopyEntryToAnotherTable() or something, but I guess this is obvious)
- for global functions it should start with noun, a name of object, or type, or context it refers to; for example: TableAddEntry, or even Table_AddEntry. This will make easier to find function you need, and function declarations will look much better when grouped by context.

Also, what about class names? Should they have "C" or "T" or some other prefix? Or be named plainly?

Quote
No file names (stored in the same location) should differ from each other solely by case.
I recently found a "funny" thing about Microsoft Visual Studio 2008 I never thought about before. It puts compiled *.obj files into one folder before linkage, hence making it impossible to have two source (*.cpp) files of the identical name even if they are placed in different directories.
I tried to find a workaround (like make it put them in relative dirs), but failed. People on the web mention proper workaround that is available only for MSVS2010.

Quote
1.10 Pointers and References
For pointers and references, the preferred convention is to have the asterisk or ampersand beside the name
You mean "before the name", right?
BTW an example code may help.

Quote
2.14 Integer Types
Using integer typedefs is must do in sake of portability!
(Ask JJS about that :))

Quote
3.1 Namespaces and using Directives
You MUST mention that example from Google CC that shows how to make an alias for namespaces!
To be honest, I never knew about that. :P

Quote
3.2 Nested Classes
...
Consider making the classes separate, or using namespaces, unless the nested class is actually part of the structure of the outer class.
This should be read as:
- if a type is to be used only internally in the other class's implementation, prefer making it nested type;
- if a type is to be used public relatively often, do not make it nested;
- special case is for types specific for number of classes. Think of STL iterators, each container has a related iterator type; and although iterators are used outside of container pretty much always, they are still made nested types simply because they have identical names (iterator, const_iterator etc).
As an additional note: remember you can always make an alias to the long name by use of typedef.
For example, intead of writing
Code: C++
  1. CMyClass::CNestedClass::FuthermoreNestedStruct  obj;
do this
Code: C++
  1. typedef CMyClass::CNestedClass::FuthermoreNestedStruct CShorterName;
  2.  
  3. CShorterName obj;
  4.  

Quote
3.4 Functions
...
Use static functions where appropriate, but don't create a new class just to group static functions together (use a namespace instead).
I think this needs some elaboration. What do you call "static function"? Do you mean just a function in appropriate namespace, or do you mean non-member function declared as static?
Code: C++
  1. static void function() {...}
  2.  
The function defined like that will be only callable from inside the object unit it was compiled in. If you try to call such function from other unit, you will get linker error.

Quote
3.5 Local Variables
...
If a variable is defined inside of a loop then it will be created and destroyed with every iteration of the loop. Particularly when working with class types this may impact performance, so it is acceptable to define the variables outside of the loop using them when it makes sense to do so.
Never ever declare an object of container or string type inside a loop! ;)

Quote
5.3 Interfaces
...
By convention, any interface classes should be named with the suffix "Interface" (e.g., "ClassInterface" or "FooInterface").
That is perhaps a case where I would strongly disagree. Adding "Interface" looks a bit lame, and it increases name length for nothing.
What about "I"-prefix? like IFoo?




Aside from all this, I believe that error handling should be discussed at the early stages of further development. This is perhaps not directly a question of code convention, but still worth mentioning.
It isn't always an easy decision. Most basic way is to return integer error code. However, there are situations when having more data along with result code could be useful.
I can name three general options that are well known to me. They all have similar traits though and have some advantages and disadvantages.
1. Return integer error code, but at the same time store additional data in error-handling system. That data will be available for reading by explicitly calling some GetLastError() function.
2. Use plain class object instead of integer as a return value, that may be simply copied and so passed outside nested functions. The object may contain error code, some auxillary data, or even text message. Unless said not to, it may pass this information to the error-handling system on self-destruction; the system will decide if to show any message to the user, write it to the log, or keep silence.
3. Return "error handler", which is declared as a pointer to integer error code, but de facto points to an extendable struct, managed by error-handling system. That handle may be used to get simple result code or cast to struct pointer to get auxilary data.
Or we may use a combo of these.
In any case I believe that
a) error handling must be uniform so far as possible,
b) it is better to have some kind of "error-handling" system (a singleton of some type) that would manage error data and messaging; I think it would be unwise to allow each function to decide what to do with error, since that will not only increase code size and make program vulnerable to non-consistent handling methods, but also counter any attempt to set up a global behavior for error messages.
« Last Edit: 30 Jun 2012, 23:35 by Crimson Wizard »

Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #4 on: 30 Jun 2012, 23:37 »
Quote
1.1 Spaces vs. Tabs [top]

Tab breaks should not appear in the source files, and should be converted to space characters. Each level of indentation should increase by 2 spaces. You should set the preferences for indentation appropriately in your editor. For other uses of whitespace see 1.3 Whitespace.

I do not agree, leading tabs are superior and won't screw up on people with different tab sizes, further alignment can still be done with whitespaces.
But even if you had to forcibly use whitespaces, I'd advise to use at least 3 spaces. 2 is too few.
No seriously 2 is too few, at least 3 or 4!

- Alan

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #5 on: 30 Jun 2012, 23:48 »
I do not agree, leading tabs are superior and won't screw up on people with different tab sizes
Quote opposite - they will screw up text on people with different tab sizes, simply because the indentation will look different and not always aligned same way in all text editors.

For example, type this in notepad using tabs as indents and alignment chars:
Code: C++
  1.         int integer_var_with_long_name  = 10;
  2.         int integer_var                 = 5;
  3.  
Now copy this to Visual Studio and see what will happen.
After, try the same with spaces.
« Last Edit: 30 Jun 2012, 23:49 by Crimson Wizard »

monkey0506

  • AGS Project Tracker Admins
  • Tasting the banhammer. Strangely, tastes like ham.
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #6 on: 01 Jul 2012, 00:44 »
Some thoughts on all this.
Please keep in mind, that many points in code conventions are usually stated in sake of consistency rather than better design or optimization.
For these I'll simply mention alternative arguments.

Of course. That's what I'd expect. ;)

Quote
1.3 Braces
"Curly braces", { and }, should occupy a line by themselves (trailing comments are acceptable also). They should be at one level of indentation lower than the code they contain. This helps as a vertical guide (in addition to the indentation) in finding where a particular block of code starts or ends.

I used this style for a very long time, until one day I was asked by our lead dev to put opening brace on the same line as "if" and "for".
He explaned that thus more actual code lines will fit in the screen. At first that may seem a bit ridiculous, but not after you spent months studying hungreds of thousands of lines in 10+ years-old code :)
On other hand, that depends on average function lengths. If we are going to stick to writing smaller functions this may be not that important.

My personal instinct is to stand by what I've written, but I'll be willing to follow if the trend is against me. Personally I find that the code itself is more readable this way because it's significantly easier to match braces (especially if indentation is changing due to copypasta or other reasons).

I did not notice if you've mentioned a rule of making each "if" and "for/do/while" contents be enclosed even if they consist of one line. This is sometimes suggested for it would be harder to make a mistake and add new line, forgetting to enclose it in brackets.

I didn't specify that, but it would be a good thing to include. I personally don't mind having single statements if they're on the same single line, but especially in broader projects with more coders involved, this would be a good suggestion, again if only for consistency's sake.

Quote
1.5 Naming Conventions
...
Local variables (inside of functions) can be named using either lower camelCase or c_style_notation, but should remain consistent throughout any single file.

Personally I like c_style_notation for function parameters and local vars more for some unexplainable reason :)
Also, if class private data is named using "lower" camelCase, using c_style_notation for locals will help to distinguish them.
There could be an interesting case though if private member's name consists of one word; in that case it would be indistinguishable from local vars. There may be options how to solve this, for example add an extra char as either prefix or postfix.

This is a fair point, as collision between member variables and locals is ugly at best. I'm not particularly a fan of prefixing all members with '_' (or what have you, others like "m_" are worse IMO), but it would be better than just letting the collision occur and end up referencing the wrong variable because a member has been masked by a local. Probably good to have members as _camelCase.

Regarding function names. There are two main options for how to form a function name, I've seen both being suggested in the web -
a) verb first, e.g. AddTableEntry();
b) noun first, e.g. TableAddEntry().
Personally I suggest this:
- for class members it should start with verb, and should not include owning class name; for example if we have class Table, member function should be AddEntry and not AddTableEntry. (Except if the function is supposed to deal with other object, like CopyEntryToAnotherTable() or something, but I guess this is obvious)
- for global functions it should start with noun, a name of object, or type, or context it refers to; for example: TableAddEntry, or even Table_AddEntry. This will make easier to find function you need, and function declarations will look much better when grouped by context.

This. Absolutely. Generally global methods should be avoided anyway, but perhaps in that case we could just take advantage of the namespace itself: Foo::AddBar rather than Foo::FooAddBar or Foo::AddFooBar. And I certainly agree that class methods shouldn't redundantly state the class name except in cases like you've shown.

Also, what about class names? Should they have "C" or "T" or some other prefix? Or be named plainly?

I don't think classes themselves need any particular prefix, but if we do decide to use one, I'd prefer using 'C'.

Quote
No file names (stored in the same location) should differ from each other solely by case.

I recently found a "funny" thing about Microsoft Visual Studio 2008 I never thought about before. It puts compiled *.obj files into one folder before linkage, hence making it impossible to have two source (*.cpp) files of the identical name even if they are placed in different directories.
I tried to find a workaround (like make it put them in relative dirs), but failed. People on the web mention proper workaround that is available only for MSVS2010.

That's interesting. Perhaps in best interest we should avoid having similarly named files altogether...but that almost predicates a foreknowledge of every file in the project. Perhaps file names should be prepended with the directory name? Seems redundant, but it would prevent collisions like this (which might not even manifest on compiler A but might explode on compiler B).

Quote
1.10 Pointers and References
For pointers and references, the preferred convention is to have the asterisk or ampersand beside the name

You mean "before the name", right?
BTW an example code may help.

Agreed that this wording might not have been the best. What I was getting at was:

Code: C++
  1. int *doThis; // preferred
  2. int* maybeThis;
  3. int * neverThis;

Quote
2.14 Integer Types

Using integer typedefs is must do in sake of portability!
(Ask JJS about that :))

"Using integer typedefs"...? From one system to the next, the actual width of the integers being used shouldn't need to change, should it? For example, I know he's recently been working on a 64-bit port, but does that actually use 64-bit values, or is it just correcting the alignment of variables that were assumed to be 32-bit (and would be 32-bit on a 32-bit system) but are actually 64-bit (on a 64-bit system)? My basic understanding is that the width of the variables themselves should be entirely consistent from one port to the next...? So, doing some conditional compilation based on the port being built we could typedef a single int type that would always be 32-bit, even on 64-bit systems. Or is that not how it works, and the width of the variables being used should be allowed to change based on target architecture?

Quote
3.1 Namespaces and using Directives

You MUST mention that example from Google CC that shows how to make an alias for namespaces!
To be honest, I never knew about that. :P

Yes, that is interesting. Functionally equivalent to a typedef for namespaces. Could be useful with longer (or even nested) namespaces especially.

Quote
3.2 Nested Classes
...
Consider making the classes separate, or using namespaces, unless the nested class is actually part of the structure of the outer class.

This should be read as:
- if a type is to be used only internally in the other class's implementation, prefer making it nested type;
- if a type is to be used public relatively often, do not make it nested;
- special case is for types specific for number of classes. Think of STL iterators, each container has a related iterator type; and although iterators are used outside of container pretty much always, they are still made nested types simply because they have identical names (iterator, const_iterator etc).

I would actually say that in the case of iterators, that falls under the category of being part of the outer class's structure. Outside the context of the outer class, what does the nested iterator mean? It could perhaps be reworded, but if the inner class only has meaning within the context of the outer class, then it's likely a good candidate to be a nested class (regardless of how regularly it's accessed).

As an additional note: remember you can always make an alias to the long name by use of typedef.
For example, intead of writing

Code: C++
  1. CMyClass::CNestedClass::FuthermoreNestedStruct  obj;

do this

Code: C++
  1. typedef CMyClass::CNestedClass::FuthermoreNestedStruct CShorterName;
  2.  
  3. CShorterName obj;

Of course...does that specifically need to be noted though? I suppose it wouldn't hurt to make mention of it just as a friendly reminder.

Quote
3.4 Functions
...
Use static functions where appropriate, but don't create a new class just to group static functions together (use a namespace instead).

I think this needs some elaboration. What do you call "static function"? Do you mean just a function in appropriate namespace, or do you mean non-member function declared as static?

Code: C++
  1. static void function() {...}

The function defined like that will be only callable from inside the object unit it was compiled in. If you try to call such function from other unit, you will get linker error.

I actually had literally no idea that this was possible. So yes, this probably needs to be reworded taking this into account, and clarifying what I meant. ;)

Quote
3.5 Local Variables
...
If a variable is defined inside of a loop then it will be created and destroyed with every iteration of the loop. Particularly when working with class types this may impact performance, so it is acceptable to define the variables outside of the loop using them when it makes sense to do so.

Never ever declare an object of container or string type inside a loop! ;)

Google's CC left that one kind of open to interpretation, so I wasn't sure if I should be more authoritative about that. Certainly makes sense to me to be a bit stricter about what can be defined inside of a loop.

Quote
5.3 Interfaces
...
By convention, any interface classes should be named with the suffix "Interface" (e.g., "ClassInterface" or "FooInterface").

That is perhaps a case where I would strongly disagree. Adding "Interface" looks a bit lame, and it increases name length for nothing.
What about "I"-prefix? like IFoo?

I actually did start to draft it as "use an 'I' prefix" but then I was just looking over Google's CC and decided that maybe that would make it a bit clearer. I'm okay with either way, but I think I do lean toward the method that I didn't say to use (so long as everyone else is okay with that). :P

Aside from all this, I believe that error handling should be discussed at the early stages of further development. This is perhaps not directly a question of code convention, but still worth mentioning.
It isn't always an easy decision. Most basic way is to return integer error code. However, there are situations when having more data along with result code could be useful.
I can name three general options that are well known to me. They all have similar traits though and have some advantages and disadvantages.
1. Return integer error code, but at the same time store additional data in error-handling system. That data will be available for reading by explicitly calling some GetLastError() function.
2. Use plain class object instead of integer as a return value, that may be simply copied and so passed outside nested functions. The object may contain error code, some auxillary data, or even text message. Unless said not to, it may pass this information to the error-handling system on self-destruction; the system will decide if to show any message to the user, write it to the log, or keep silence.
3. Return "error handler", which is declared as a pointer to integer error code, but de facto points to an extendable struct, managed by error-handling system. That handle may be used to get simple result code or cast to struct pointer to get auxilary data.
Or we may use a combo of these.
In any case I believe that
a) error handling must be uniform so far as possible,
b) it is better to have some kind of "error-handling" system (a singleton of some type) that would manage error data and messaging; I think it would be unwise to allow each function to decide what to do with error, since that will not only increase code size and make program vulnerable to non-consistent handling methods, but also counter any attempt to set up a global behavior for error messages.

I kind of lean toward #2 here. I definitely agree that defining a consistent error handling mechanism early on will make things far easier later on down the road. And the sooner the better. I think that if we did go with method #2 (for example) that we could add a coding convention that any class must implement an Initialize method that returns a pointer to the error handling class (NULL for none). Certainly warrants further discussion.

[Edit:] Dang post ninjas. Two spaces might be considered too few, but I'd say we should keep it at even numbers (read as: 2 or 4, anything more than 4 is just stupid :P). Oh, and I will maintain that tabs shouldn't exist in code. ;)
User was banned for this post.

RickJ

  • fix'n one thing and break'n two ...
    • I can help with scripting
    •  
    • I can help with story design
    •  
    • RickJ worked on a game that was nominated for an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #7 on: 01 Jul 2012, 05:27 »
Nice job monkey, very nice job.  The others make some good points to consider.  The final draft of course ought to reflect the consensus of the people doing the work.  Adopting a consistent set of conventions that everyone can live with is more important than adopting the "absolutely most best out standing standard", ;) because that one probably doesn't exist; though you have come pretty close.  This is just my caution to everyone to not get all religious  about this ;D.

Here are a couple of my opinions on a couple of issues:

1.  I prefer the c_style_notation for parameters and local names.

2.  Curly braces {} I have used both styles (opening brace on same or next line) and don't think one is better than the other; it's more a question of personal preference.

3.  I agree with monkey about pointers, i.e. int *doThis; // Preferred

4.  Indent of 2 spaces is too small.  Btw what's the deal with even numbers here?  It doesn't matter but I'm just curious.

5.  Empty Loops - Presumably empty loops are place holders for future code.  In this case wouldn't it be better to document why the loop is empty?" 
Code: C
  1. while (false) {
  2.     // Future stuff goes here ...
  3. }
  4.  

6.  Block Comments /* */ - I agree with monkey.  If they are not normally used then they could be used during testing to comment out whole sections of code.

7.  Block or Banner Comments vs end of line comments -  I think there needs to be some discussion on when/how to use each ..

I think you guys are all doing a great job.  Keep up the good work...

JJS

  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2012, for his efforts in porting AGS to multiple platforms
    •  
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #8 on: 01 Jul 2012, 07:33 »
About the fixed types: There are instances when you absolutely need a variable size type, e.g. in the script engine. The registers of the virtual machine must be able to hold pointer values so they have to be 64 bit or 32 bit. On Unix-style systems this can be done by declaring them as long int (that is what is I am using right now) but on Windows a long int is always 32 bit and only a long long int is 64 bit. So for greatest portability the variable must be defined as an intptr_t.
Ask me about AGS on PSP, Android and iOS! Source, Daily builds

Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #9 on: 01 Jul 2012, 08:21 »
I do not agree, leading tabs are superior and won't screw up on people with different tab sizes
Quote opposite - they will screw up text on people with different tab sizes, simply because the indentation will look different and not always aligned same way in all text editors.

For example, type this in notepad using tabs as indents and alignment chars:
Code: C++
  1.         int integer_var_with_long_name  = 10;
  2.         int integer_var                 = 5;
  3.  
Now copy this to Visual Studio and see what will happen.
After, try the same with spaces.

Done, VS doesn't seems to mind if I use
-> -> ->int integer_var...........= 5;

Of course if I try to paste in an identation not aligned to tabs, then in that case it converts the tab to spaces, gotta be the "smart indenting".
I like tabs because one tab is one tab regardless of tab sizes. Well, yes, I may be partial to it due to being used to C# with other projects.
Still I hope you pick either 3 (the minimum for readability) or 4 (better) as tab size.


I agree with Crimson Wizard about interfaces, I too prefer the "I" prefix.

Regarding local variables inside loops, declaring them inside is actually faster, at least for value types. I found out while experimenting with software filters.


This is a fair point, as collision between member variables and locals is ugly at best. I'm not particularly a fan of prefixing all members with '_' (or what have you, others like "m_" are worse IMO), but it would be better than just letting the collision occur and end up referencing the wrong variable because a member has been masked by a local. Probably good to have members as _camelCase.

I support prefixing members with '_' (yes 'm_' is ugly and deprecated).


- Alan
« Last Edit: 01 Jul 2012, 08:34 by Alan v.Drake »

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #10 on: 01 Jul 2012, 11:45 »
5.  Empty Loops - Presumably empty loops are place holders for future code.  In this case wouldn't it be better to document why the loop is empty?" 
I know I am probably nitpicking, but empty loops may sometimes be used when loop header contains implementation on its own. Just a random example:
Code: C++
  1. for (int i = 0; i < ptr_array.size && ptr_array[i] != NULL; ++i) ; // no code inside
  2.  
(finding the first free slot in the pointer array)

Quote from: Alan v.Drake
Done, VS doesn't seems to mind if I use
-> -> ->int integer_var...........= 5;
I mean using sole tabs. Combining tabs and spaces is cheating :)... no, wait, what I mean - this requires using two techniques when you could use one.

Quote from: Alan v.Drake
Of course if I try to paste in an identation not aligned to tabs, then in that case it converts the tab to spaces, gotta be the "smart indenting".
I like tabs because one tab is one tab regardless of tab sizes. Well, yes, I may be partial to it due to being used to C# with other projects.
I am starting to think we are speaking about different things... because I am not sure I understand what you mean.
You tell that "tab is one tab" regardless of tab size. But that does not make much sense. Do you mean that, on the contrary, space is not always one space?  (wtf) Or 2 (4) spaces are not always 2 or 4 spaces? I didn't get this argument.
You mentioned C#. But as far as I know, MSVS default settings for C# code actually is to insert spaces instead of tabs when you hit Tab (at least that's how things are in my Studio installation).

On a side note I don't care much about number of spaces. 2,4 - I can use any.

Quote from: Alan v.Drake
Regarding local variables inside loops, declaring them inside is actually faster, at least for value types. I found out while experimenting with software filters.
I am not sure how that is possible... like declaring ONE "int a" before the loop is slower than declaring ONE "int a" inside the loop?
In C++ practice, for example, if you are using array class in loop, declaring it each iteration would mean there's memory deleted and re-allocated every pass, which, depending on number of iterations and how often this code is called, may result in BIG slowdown. On other hand, if you declare array object before the loop and use only it (resetting contents) it may be faster, since the array class (if it is correctly written) won't actually delete the memory it uses, but rather reuse what it already has.
« Last Edit: 01 Jul 2012, 11:55 by Crimson Wizard »

Mazoliin

  • Now how do I get down...
    • Mazoliin worked on a game that was nominated for an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #11 on: 01 Jul 2012, 12:10 »
I don't know much about it, but prefixing variables only with '_' isn't all that safe, as it's for reserved identifiers, same goes for '__' (double underscores).

Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #12 on: 01 Jul 2012, 16:01 »
Quote
Quote
Done, VS doesn't seems to mind if I use
-> -> ->int integer_var...........= 5;
I mean using sole tabs. Combining tabs and spaces is cheating :)... no, wait, what I mean - this requires using two techniques when you could use one.
They're both viable ways, I guess.

Quote
I am starting to think we are speaking about different things... because I am not sure I understand what you mean.
You tell that "tab is one tab" regardless of tab size. But that does not make much sense. Do you mean that, on the contrary, space is not always one space?  (wtf) Or 2 (4) spaces are not always 2 or 4 spaces? I didn't get this argument.
You mentioned C#. But as far as I know, MSVS default settings for C# code actually is to insert spaces instead of tabs when you hit Tab (at least that's how things are in my Studio installation).
Yeah, I did express myself poorly, I meant that that a tab indentation always corresponds to the tab size, while with spaces it may or may not, depending on the user prefs.
Spaces might be the default on VS, but the sources I've been working on so have always been with tabs, so I got used to them (I code for a RunUO shard, most of what I know of C# comes from there. Oh, ok, it makes me a little sad that those filthy spaces are winning against my beloved tabs :( ).

Quote
I am not sure how that is possible... like declaring ONE "int a" before the loop is slower than declaring ONE "int a" inside the loop?
In C++ practice, for example, if you are using array class in loop, declaring it each iteration would mean there's memory deleted and re-allocated every pass, which, depending on number of iterations and how often this code is called, may result in BIG slowdown. On other hand, if you declare array object before the loop and use only it (resetting contents) it may be faster, since the array class (if it is correctly written) won't actually delete the memory it uses, but rather reuse what it already has.
Don't ask me, I only know that when I moved them out I saw a loss of 3 fps, so I put them back inside the cycle. I could have fucked up something, or there might have been compiler optimizations involved or something.


- Alan

RickJ

  • fix'n one thing and break'n two ...
    • I can help with scripting
    •  
    • I can help with story design
    •  
    • RickJ worked on a game that was nominated for an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #13 on: 01 Jul 2012, 22:27 »
@Crimson:  I agree with your point, and have done similar things in the past.   But since it actually does something I didn't think of it as being empty ... although I suppose it actually is.  The only thing I (and presumably you also) would change in the example is the eol comment.
Code: C
  1. for (int i = 0; i < ptr_array.size && ptr_array[i] != NULL; ++i) ; // find first free slot in pointer array
  2.  


Wyz

  • AGS Project Tracker Admins
  • anno 1986
    • I can help with making music
    •  
    • I can help with story design
    •  
    • I can help with translating
    •  
    • I can help with voice acting
    •  
    • I can help with web design
    •  
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #14 on: 02 Jul 2012, 02:35 »
Good job on the code conventions there monkey I must say. I'm generally very prissy about these and I quite like them. I haven't had time to read all responses unfortunately but I'll comment regardless.
Personally I really like Allman bracket style so I'm happy to see that being part of your proposal. :-D
A bit on numeric types and 64 bit support: what most engines do and AGS should too is define custom types somewhere using a macro or typedefs, this makes it a lot easier when porting to different platforms. Macros have the added bonus that they can be redefined in platform specific make files.
A bit about virtual classes and RTTI: it should not be uses unless the polymorphic properties are a necess. They are very much not portable: imagine having the engine compiled with MSVC and a plugin with MinGW, that will break horribly as I came to know the hard way. ;)
Finally I'd like to put a vote in for using tabs instead of spaces; spaces tend to go really messy. On the other hand spacing with tabs can be adjusted accordantly and this can even put in the convention.
Anyways, thanks for putting this out there!

Ps.
Forgot to mention and does not really belong in this thread but still: getters and setters are inherently silly. I've seen people make functions just to retrieve and set a private member; a load of crap if you ask me.
« Last Edit: 02 Jul 2012, 03:57 by Wyz »
Life is like an adventure without the pixel hunts.

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #15 on: 02 Jul 2012, 10:24 »
Couple of things I forgot to mention.

Quote from: monkey_05_06
Quote from: Crimson Wizard
I recently found a "funny" thing about Microsoft Visual Studio 2008 I never thought about before. It puts compiled *.obj files into one folder before linkage, hence making it impossible to have two source (*.cpp) files of the identical name even if they are placed in different directories.
I tried to find a workaround (like make it put them in relative dirs), but failed. People on the web mention proper workaround that is available only for MSVS2010.

That's interesting. Perhaps in best interest we should avoid having similarly named files altogether...but that almost predicates a foreknowledge of every file in the project. Perhaps file names should be prepended with the directory name? Seems redundant, but it would prevent collisions like this (which might not even manifest on compiler A but might explode on compiler B).
I think that's too much for such a rare problem. I think it would be better to resolve this by finding a compromise and name them differently.


Quote from: monkey_05_06
4.0 Header Files [top]

Every header file should have predefined macros to prevent multiple inclusion. The preferred naming convention for these macros is two underscores, optionally the path followed by another underscore, the file name, underscore, and a trailing capital letter 'H'.
Just FYI, when refactoring I introduced following header guard template:
Code: C++
  1. __AGS_EE_GUI__CSCIDIALOG_H
  2.  
EE stands for EnginE and; I also used CN for CommoN lib.
GUI is "module" or "part" name, I also used AC for general stuff, AUDIO for audio code etc.
CSCIDIALOG is literally a name of file.
We may use this, or come with something else.



Quote from: WyZ
Finally I'd like to put a vote in for using tabs instead of spaces; spaces tend to go really messy. On the other hand spacing with tabs can be adjusted accordantly and this can even put in the convention.
Heh. ;)
Spacing with tabs can be adjusted accordantly to insert spaces instead of tabs. The question here is not what key to press, but what characters to put into text. I personally use this all the time, I am not a psycho to manually type 4 spaces each time I need a newline (or even more if I need complex indentation).
« Last Edit: 02 Jul 2012, 10:33 by Crimson Wizard »

monkey0506

  • AGS Project Tracker Admins
  • Tasting the banhammer. Strangely, tastes like ham.
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #16 on: 03 Jul 2012, 06:25 »
Good job on the code conventions there monkey I must say. I'm generally very prissy about these and I quite like them.

Yes, for the most part it does seem that the proposal is holding up pretty well to most everyone's liking (a few things here and there are to be expected I imagine). Glad to have appeased your tastes! ;)

A bit on numeric types and 64 bit support: what most engines do and AGS should too is define custom types somewhere using a macro or typedefs, this makes it a lot easier when porting to different platforms. Macros have the added bonus that they can be redefined in platform specific make files.

Macros can easily lead to ugly code, so it would be nice to have this wrapped up in one of the lower-level source files (probably some sort of "Common" header to be included). Of course I'm sure that's what you meant as well.

A bit about virtual classes and RTTI: it should not be uses unless the polymorphic properties are a necess. They are very much not portable: imagine having the engine compiled with MSVC and a plugin with MinGW, that will break horribly as I came to know the hard way. ;)

Well RTTI itself is currently prohibited by the current conventions (as the proposal stands). Are there other caveats to using polymorphic classes that I might have overlooked? So long as you're not using RTTI, does polymorphism itself present a portability problem?

Finally I'd like to put a vote in for using tabs instead of spaces; spaces tend to go really messy. On the other hand spacing with tabs can be adjusted accordantly and this can even put in the convention.

The general consensus still seems (to me at least) to lie with using spaces rather than tabs. I absolutely disagree that "spaces tend to go really messy". I think that as a programmer I should have enough pride in my work to prevent such things from ever happening. But seriously, I don't think we should argue about the matter, as there are pros and cons to each. I think that moving forward, so long as everyone can agree to follow the common trend (whatever the case may be) then we'll be good (hence the conventions ;)). (Personally though, I find tab breaks to be absolutely horrid to work with...)

Forgot to mention and does not really belong in this thread but still: getters and setters are inherently silly. I've seen people make functions just to retrieve and set a private member; a load of crap if you ask me.

While I can agree that "empty" getters and setters (meaning they only have a return statement or assignment without any other logic, etc.) are just adding overhead unnecessarily, there are legitimate reasons why someone would want to be able to have greater control over what happens when the members are accessed (obviously). In those cases, accessors make sense, but I will agree that they shouldn't be considered a requisite if there's no logic being applied. I will consider rewording that portion of the conventions.

5.  Empty Loops - Presumably empty loops are place holders for future code.  In this case wouldn't it be better to document why the loop is empty?"

I know I am probably nitpicking, but empty loops may sometimes be used when loop header contains implementation on its own. Just a random example:

Code: C++
  1. for (int i = 0; i < ptr_array.size && ptr_array[i] != NULL; ++i) ; // no code inside

(finding the first free slot in the pointer array)

Per the current draft of the conventions though, such an empty loop should have braces or a continue statement, rather than just the semicolon at the end. Nitpicking, I know, but that in itself can act as a form of self-documentation, and also help prevent any confusion with while statements (that they may be the end of a do-while statement).

I don't know much about it, but prefixing variables only with '_' isn't all that safe, as it's for reserved identifiers, same goes for '__' (double underscores).

C++'s scope resolution protects against this in the case of class member variables, which is what we were discussing having prefixed. The C++ standard reserves any name starting with a single underscore (not followed by another underscore or a capital letter) for the implementation (i.e., standard libraries), but only within the global namespace. Since we are specifically avoiding placing things into the global namespace to begin with, this is a non-issue altogether. Definitely thanks for the feedback, as I did have to look into it ;), but turns out that it's not a relevant concern.
User was banned for this post.

Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #17 on: 03 Jul 2012, 21:07 »
Is there a format to follow for our notes inside comments ?
// [Username] 2012-12-31: Sausages
// blablabla - Username
// something else ?

- Alan

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #18 on: 03 Jul 2012, 21:18 »
Is there a format to follow for our notes inside comments ?
// [Username] 2012-12-31: Sausages
// blablabla - Username
// something else ?

- Alan
I agree.
I usually used [IKM] (my initials) or [CW] (not anymore).

Crimson Wizard

  • Local Moderator
  • AGS Project Tracker Admins
    • Best Innovation Award Winner 2013, for spearheading the AGS 3.3.0 project
    •  
    • Lifetime Achievement Award Winner
    •  
    • Crimson Wizard worked on a game that was nominated for an AGS Award!
      Crimson Wizard worked on a game that won an AGS Award!
Re: AGS Engine Coding Conventions - Proposal Draft 1.0
« Reply #19 on: 09 Jul 2012, 08:51 »
I have a question regarding copyright/license info that is put at source file headers. I don't know about that stuff much. Should we keep those texts from CJ's code ("Copyright (C) 2000-2005, Chris Jones" etc), or write totally new ones?