string parsing crashes

All code submission.
mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Sat Oct 02, 2010 11:24 am

1) opening/closing braces?
I prefer putting them on a new line. It helps with alignment of code blocks in my opinion.
2) spacing between parens and parameters for declarations and calls: this is what my current dev team uses in our code.
method signatures: space between the method name and first paren.

Code: Select all

void methodA (string param1, string param2)
method call:

Code: Select all

this.methodA( "foo", "bar" );
3) single operation if statement
a) For consistency I add the braces around the operation.
b) braces should be on new line for consistency

4) indentation:
4 spaces. This is common in most modern editors (ie Visual Studio, Eclipse, etc ). It also helps with readability IMO.

5) variable naming
a) camelcase
b) no strong opinion here, just that they are descriptive and consistent.

6)max nest block depth
anything nest more than 3 levels seems to scream for refactoring

7) line size limit
Nothing concrete. We all have higher res screens these days, so restricting to something like 80 chars is a bit much. However, something that forces the reader to scroll over three pages worth is too long.

8) variable /method declaration
h files
private, protected,public > Attributes then methods > And alphabetical order inside those: wololo

cpp files
they should be grouped by scope and function as well as alphabetized. Yes modern editors are smarter about it, but visually our minds process things quicker with some order.

** variable and method names should be descriptive. Having 1 or 2 character variable names my make typing easier, but we all are using smart editors right?

9)
* Comments around methods describing what they do unless utterly simple would be extremely useful. Especially for those of us (noobs and experienced) trying to understand the code. A line here or there goes a long way.

* First class representation of data. If there is information that floats around via strings, integers, etc that can be described by encapsulating it into an object, we should do so. We shouldn't try to infer meaning by coding around it. This may be a combination of creating new classes or refactoring old ones to form new objects. Just because data is currently stored in a particular way now, doesn't mean that it is set in stone. Requirements may change or new concepts may conflict with the current representation of the data.

* I find the use of Macros a little hackish and impossible to truly debug since they are not part of the active code. I would use them sparingly if at all. I would also want them to be very simple like SAFE_DELETE. Anything more invites possibilities for buggy code down the line.

* I tend to think header files should just be header files and not contain any implementation code at all. I understand that there may be some supposed performance improvements but I think having the ability to debug more important. Again, if one must inline, keep it to something very simple like getX() returns X without any processing

* We should also be careful not to have methods do more than what they seem to do. Methods should be clear in their contract with the calling code what is going to change. if I pass in two parameters to a getter method, neither of the params should be returned modified. An accessor is a read only operation, otherwise the design is flawed.

* single return point in a method is a good idea. My team and I try to enforce this. It may seem obtuse, but it guarantees that there is only one way out of a method. I've found that it helps with the flow of the method.

* I personally like to dereference variables before I put them into a conditional. It helps with keeping line size smaller and debugging. I've noticed VS doesn't always do the right thing when you hover over a pointer of a pointer from time to time for information. This way will allow you to put a break point at the conditional and see what it is trying to evaluate.

So code like this :

Code: Select all

if ( a->card->previous )
{
 // do something
}
becomes

Code: Select all

object obj = a->card;
if ( card->previous ) 
....
of course you'd want to put in guards where appropriate to catch and deal with error conditions.

Zethfox
Posts: 3022
Joined: Thu Jun 10, 2010 11:28 pm

Re: string parsing crashes

Post by Zethfox » Sat Oct 02, 2010 5:02 pm

wololo wrote:Ok, let's hijack this topic and discuss coding guidelines, I do want us to have stricter rules after the upcoming release. I'll try to have the other devs have a look at this thread and state their own arguments.

Please answer to the following questions, give examples if you want. I'll update this thread with your choices, you're of course allowed to change your mind as we discuss

1) How should we format opening/closing brackets ?
always on a new line: MootPoint, mnguyen
always on the same line:
no opinion: zeth
On the same line except for functions opening/closing brackets: wololo

2) spacing between parens and parameters in method signatures and calls

int foo(int a, int b) { : wololo,zeth

3.a) Should we add brackets after a "if" if there's only one operation inside the if ?
No:
Yes: wololo
ALWAYSzeth
3.b) should the unique operation after a "if" be on a separate line?
No:
Yes: wololo, MootPoint,zeth

4) Indentation: 2 or 4 spaces ?
2:
4: wololo, mnguyen,zeth

5.a) variable naming: camelcase or lowercase+underscore ?
camelcase wololo,zeth
lowercase+underscore

5.b) variable naming: how to prefix global variables, class attributes, local variables?

gGlobal, mAttribute, noSpecificRuleForLocalVars : wololo

5.c) NO MORE CRYPTIC VARIBLES!!!!!! if a varible can be given a name, use a real name reflecting its function!!!


6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)

No
3: wololo
dependent on how many different abilities a function shares zeth

7) any limit to the size of a line ?
No: wololo,zeth

8) What order should we use for variable/methods to be declared in .h files? How about order in .cpp files?
h files
private, protected,public > Attributes then methods > And alphabetical order inside those: wololo
no opinionzeth
cpp files
No specific order: wololo,zeth

9) Anything else? (we can also discuss that later, we don't need all rules at once)

rule are fine and all, and i will try to follow them, however three i want to stand EXTREMELY strong on...
first)
DO NOT USE CRYPTICALLY NAMED VARIBLES! no more "int GIE" type varibles...USE real words if this varible can be named something REAL.
second)
DO NOT REUSE THE SAME VARIBLE NAMING over and over for all sorts of functions. exsample "tap" in one function it is a varible meaning if card is booled as tapped. in another instence it is used for something COMPLETELY different. if the varible would do something different in another function DO NOT REUSE "tap" call it something else.
third)
COMMENT COMMENT COMMENT!!! look at guiplay for an exsample of what NOT to do. you have to sit there and figure out each function step by step because there are no comments in there at all, none telling what varibles (cryptically named mind you) do, none telling you what the function does and in what order. if you have gone through 3 different sections of a code and havent ATLEAST added one comment SOMEWHERE, telling the reader what exactly is happening at that point, then this is directed at you.

MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

Re: string parsing crashes

Post by MootPoint » Sun Oct 03, 2010 1:37 am

1) How should we format opening/closing brackets ?
always on a new line :)

2) spacing between parens and parameters in method signatures and calls
int foo(int a, int b) is fine.

3.a) Should we add brackets after a "if" if there's only one operation inside the if ?
Yes
3.b) should the unique operation after a "if" be on a separate line?
Yes
4) Indentation: 2 or 4 spaces ?
4

5.a) variable naming: camelcase or lowercase+underscore ?
camelcase (specifically lower camelcase for local scope declared variables)

5.b) variable naming: how to prefix global variables, class attributes, local variables?
gGlobal, mAttribute, localVariable

6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)
never thought about this, but yeah, if you're over 3, refactoring is probably warranted.

7) any limit to the size of a line ?
no, except that if you have a long line, it means the code is getting harder to read. Back to my example on how to write a constructor with member initializers...

Code: Select all

FooBar::FooBar(int inValue, const std::string& inSomeString, const std::string& inSomeOtherString) : mSomeString(inSomeString), mSomeOtherString(inSomeOtherString)

ugh! instead, wrap the initializers please!

FooBar::FooBar(int inValue, const std::string& inSomeString, const std::string& inSomeOtherString)
	:
	mSomeString(inSomeString),
	mSomeOtherString(inSomeOtherString)
{
	// code goes here	
}
8) What order should we use for variable/methods to be declared in .h files? How about order in .cpp files?
h files
public, protected, private. Functions first, variables last.

cpp files
No specific order (except: Constructors/destructors/copy constructors/init functions at the top - any member functions, go nuts)

9) Anything else?
On the subject of camelcase, function names should always be upper camelcase to distinguish them from variable names (lower camelcase). Ie MyFunction() vs mFunctionPtr.
Agreed with Mike & djardin - down on Macros (as you can't step through them with the debugger), and use templates sparingly. Also agree with djardin about minimizing the number of return calls within a single function. I'm not quite so strict (ie, 2 -3 would be tolerable), but if it grows beyond that, you should be refactoring.

Just to be a little more explicit about #2):

if(something==0) vs if (something == 0) ? (note the space between if and the open parens, as well as the spaces between the operator)
I choose the second version

similarly,

for(int x=0;x<<container.size();++x) vs for (int x = 0; x << container.size(); ++x)
I choose the 2nd again. It's just a little easier on the eyes.

Jean
Posts: 32
Joined: Tue Nov 18, 2008 5:12 am

Re: string parsing crashes

Post by Jean » Tue Oct 05, 2010 2:11 pm

** 1) How should we format opening/closing brackets ?
>> Always on a new line for me.
I'm with MootPoint's example : as soon as the line with the bracket and the next line are both non-trivial (which happens a LOT in practice), it makes stuff hard to read. Then you'll see people putting an empty line after the line with the brace, which not only kills the advantages but is also even harder to read.
An alternative way of thinking about this is discourage very strongly long lines, put results of temporary computations in variables with explicit names as much as possible, and encourage taking up vertical space instead of horizontal space. That makes the whole "brace on the same line" style more readable, so if we go with braces on the same line it's something we can consider.



** 2) spacing between parens and parameters in method signatures and calls
>> int foo(int a, int b) { : I like that.
It's probably a good idea to basically follow english grammar for code. The one exception I see is the space before parens which I like to leave out, because it's much more portable across programming languages (recent languages are inclined to understand "foo (bar, qux)" as "foo((bar, qux))").



** 3.a) Should we add brackets after a "if" if there's only one operation inside the if ?
>> Certainly not as a rule.
I think it's a very bad idea to state that as a rule. Putting braces helps in a variety of cases, especially while debugging. It makes the code arguably a little harder to read once it's finished writing, even though it's not a big deal when you're used to it. Basically writing them makes stuff marginally easier to write, and marginally harder to read.
But stating it as a rule prevents us from using that for the purpose it was meant : treat boundary cases without too much fuss.
It often happens you want to set default values or reset to something sensible if some value shoots the intended range. In this case, this is not really part of the logic or algorithm, and should be as discreet as possible.

Code: Select all

if (x > 99) x = 99; // Should not shoot 99
...is much more discreet than

Code: Select all

if (x > 99) { // Should not shoot 99
  x = 99;
}
...which looks like a big part of the main treatment. No braces makes for lighter code for places where it's simple and not core functionality. It relates to "return early" policies. Lighter code is easier to read.

So yes, in the end enforcing this rule trashes readable pre-conditions and post-conditions. That's why I'm strongly against any rule about this : there are cases where it's much, much more clear without them, and cases where it's much, much more convenient with them.



** 4) Indentation: 2 or 4 spaces ?
> No strong opinion.
I lean toward the 2-end of the spectrum, but I'll be happy with 4. I'd be sad if we had 4 AND braces on a new line AND both braces and inside indented. I don't like this :

Code: Select all

if (foo)
    {
        bar();
Aside from that, 2 or 4 are fine with me.
I think we should maybe state as an additional rule that we use spaces and not tabs to indent.



** 5.a) variable naming: camelcase or lowercase+underscore ?
> CamelCase
No strong opinions on this, I'd be happy with both. I'm against underscores at start and end of identifiers though, which is more discouraged by CamelCase.



** 5.b) variable naming: how to prefix global variables, class attributes, local variables?
> No prefix
I'm against prefixing variables, because it encourages putting a lot of globals and a lot of attributes, and cripples smart completion.
If you think about it :
- Good functions are short, so you see local variables very quickly. Type coloration makes them quite obvious.
- Globals, we of course want to avoid. Most any global should be constant, so it will usually start with a capital letter. Or, you'll have a singleton, so there again capital.
- Attributes, we want to avoid too. It's state, and state makes stuff harder to debug. I know it's harder to avoid them in C++ where there are no closures (amg no closures ;_;), but whenever reasonably possible, state should be avoided.

One important point about prefixing variables is that it goes against smart completion (we all use smart editors don't we ?). When you think of a variable, the first think you think of is its meaning. Prefixing them means you have to go through the whole process of imagining whether it's an attribute or not, by which time you remembered the name in whole. Modern completion works best with names decided by semantic and not by syntax.



** 6) any maximum number of nested blocks depth? (this is of course flexible..it means the coder will be strongly encouraged to refactor when this happens)
> No.
Though I think 3 is a reasonable average, and I'd live with it. It's a nice rule of thumb, but I see little point in stating it as a coding standard. If you have someone dumb enough to write a 5-nested unreadable loop when there are better ways to write it, I do not think the rule will make that person rewrite it in a more sensible way - they'll probably just make huge functions with a lot of calls, try to unroll the loops and generally deal with it in a syntactic manner, which won't help at all.


** 7) any limit to the size of a line ?
> No.
Though, same as before, it may be discouraged to have lines over 80 or 100 characters.
One of the dangers of stating this as a rule is having people just insert line breaks. Which can be good sometimes, but there are times when we're better off writing it another way. I know I'm inclined to do that and have huge tests when I'm *** because I don't want to be moving code around when I'm in a hurry, and that sucks.
Anyway it's hard to state a good general rule about this. Consider the following case :

Code: Select all

if (((x > rect.left && x < rect.left + rect.width) || -1 == rect.width) &&
    ((y > rect.top && y < rect.top + rect.height) || -1 == rect.height))
{ ... }
Compare to :

Code: Select all

const bool widthDoesntMatter = (-1 == rect.width);
const bool heightDoesntMatter = (-1 == rect.height);
const bool withinXBounds = widthDoesntMatter || (x > rect.left && x < rect.right);
const bool withinYBounds = heightDoesntMatter || (y > rect.top && y < rect.bottom);
if (withinXBounds & withinYBounds)
{ ... }
The first one is easier to write, but with coloration the second one is (arguably ?) easier to read.

Compare :

Code: Select all

if ((WND_FLAG_TRANSIENT | winFlags)
    || (WND_FLAG_NO_BORDER | winFlags)
    || (WND_FLAG_HIDDEN | winFlags)
    || (WND_FLAG_STRANGEMODE | winFlags))
{...}
And :

Code: Select all

const bool transient = WND_FLAG_TRANSIENT | winFlags;
const bool noBorder = WND_FLAG_NO_BORDER | winFlags;
const bool hidden = WND_FLAG_HIDDEN | winFlags;
const bool strangeMode = WND_FLAG_STRANGEMODE | winFlags;
if (transient | noBorder | hidden | strangeMode)
{...}
...I don't see much improvement in writing it the second way, on the contrary. If the first way bothers you, you may as well write a function or a macro to shorten it (local, if possible).

So conclusion : I don't think there is any general rule about this that won't do as much harm as it will do good.



** 8) What order should we use for variable/methods to be declared in .h files? How about order in .cpp files?
> Semantic grouping
I'm strongly against syntactic grouping, it makes for meaningless ****. A striking example is the fact the order of declaration in a class defines initialization order. You'll have conflicting cases.
Also, to give an example of how much suxxance that such a rule yields, I'd rather much :

Code: Select all

class ... {
...
int height();
int width();

int trashable();
}
...rather than :

Code: Select all

class ... {
...
int height();
int trashable();
int width();
}
...this is bad enough like this. Imagine that when you have 40 member functions.
So, same stuff : rule of thumb why not, iron rule no. It's much better to try to make sense. Use alphabetical order when it doesn't matter. Let documentation automatic tools do the automatic ordering.



** 9) Anything else? (we can also discuss that later, we don't need all rules at once)
A number of little things. Suggestions mind you, so that we can think about it, not really rule propositions.
- Maybe we should discourage private in favour of protected. I've run into too much code where the original developer put "private" because it was seen as good practice, but it totally prevents you to do anything good with a subclass. So, protected all the way.
- I like to write constants first. "if (null == foo)" is better than "if (foo == null)". We all had bugs with single equal instead of double, this is a way to make things better.
- "++i" is better than "i++" because it doesn't cause a side effect. I've seen too many perverse and awfully undetectable bugs with i++ that wouldn't have existed if ++i had been the standard way to write it.

Also a little reminder : while it's nice to think about rules, we should consider what happens when we edit code and fix bugs. The prime example is writing comments. An innocent function like this :

Code: Select all

void enstackBlocker(Blocker *foo) { stack->push(blocker); }
...would look silly with any more comment.
But when you realize you have to test whether the stack is created or not, you'll add a null test. And that doesn't need any more comment. And then you'll have two stacks, one for self blockers and one for opponent blockers, based on some attribute, and you'll write a 2-line test. Weeks later, you'll add a line to test color. Weeks later, you'll add a line for destination. None of these changes warrant a comment : all of them are one-liners or close, very explicit, very simple. But the function itself will start looking like a Frankenstein monster.
In this case, there is never any incentive to write a new comment. Actually, adding new lines will make the logic of old lines harder to grasp, but not the logic of what you are writing, so that stating "ADD COMMENTS !" as strongly as you want will not help at all. And when you write new code, you're often not the one who wrote the lines that now need comments, and may not know why and how they where inserted there, which even lowers the incentive to comment.

While it may sound silly, maybe we want to think of a rule that would look like "add comments as soon as the function is over 4 lines", which would prompt someone who adds "obvious" code to comment older code that became not-so-obvious while it's still easy to understand. Also "don't use words that are part of the function name" to alleviate the redundant comments, though I'm not sure that really helps with anything at all.


*** Other

@ MootPoint :
What if I want to put a breakpoint on any of these error conditions? Uh, I can't.
Uh, I'm not convinced we should base our coding standards on the shortcomings of some particular IDE.
I mean, I get your point. It's best to have rules that match our environments. But if someone out there comes that writes code in notepad, and then claims we should have shorter variable names because it's easier to type, I think we'll agree to ask them to switch editors. My point is : we don't want to reduce readability to workaround some weak IDE defect.


@ Jeck :
I find it immensely helpful to have a single sentence synopsis comment following function member definition. Not as a hard rule, but as a strong suggestion? Really, the more comments the better, in my opinion.
While I like comments as much as the next guy, I'm not sure this particular rule helps. If you want a one-line description, the function name is probably the best place in the world to be explicit. I despise the following kind of comment :

Code: Select all

// Get height
int getHeight();
// Get width
int getWidth();
...and I think this rule encourages that. It's not only not helpful, it's actually in the way.

Maybe however we could encourage it for *variables*. It's something I've thought about for a long time, haven't seen it implemented ever, and wonder if it may have good results. Though the variable name is the good place to be explicit about its meaning, maybe its expected bounds, null behavior and all might benefit from being written there.


@ DJardin :
Any restriction on the usage of template and macro ? They can make the one writing them very proud but they can also make the ones using or modifying them very unhappy depending of their complexity ...
Hard to tell. Historically macros have been used for most horrible things. However they sometimes come in handy for cosmetic reasons and make the code look much more beautiful and easier to read than could be done without them.
Maybe we could exclude macros in .h files and require some specific naming scheme for them ? Keep them in the .cpp file for locality, and if you want a specific set of macros for a specific set of source files, group them in an appropriately-named .macro file that you include ? I don't know, just throwing ideas.

Anyway it's clear that macros are often ill-used and it's hard to make them safe. Any r-value macro should be enclosed in (), any non-rvalue macro should be enclosed in "do ... while false" if you really want to avoid any strange side effects with surrounding code... and then you still have your back open to parameters used several times or such. When applicable at all, we should encourage inline static functions instead of macros.

For templates it's harder to state. They don't have the same history of unskillful use, and they are such a powerful, versatile and useful tool that any limitation may get in the way of smarter code.

return point in method. My company forces every method to have a single return point
I don't think that's great. Returning early is a nice way of getting corner cases out of your brain, and often makes for much lighter code.

Code: Select all

if (not_applicable) return; // And don't care any more
I mean, I see your point : it's basically the same as goto being considered harmful, and it has solid grounds - especially when considering habits that also work with newer, better languages. On the other hand, early return sometimes get you gracefuly out of intricate nestings, and I think that especially in C++, that takes it.


@ mnguyen :
First class representation of data.
Very strongly in favor of this.
I tend to think header files should just be header files and not contain any implementation code at all.
The one problem with that is templates. C++ has a number of shortcomings, and one of them is template instanciation needing full source. There are precompiled headers to deal with that but no real unification about them. And templates is much too useful a tool to shun because of such a (relatively to the use of templates) small reason.
We can however decide to ask for templates to go in a different kind of files.

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Sun Nov 14, 2010 2:54 am

Here is a draft, based on this discussion, and the existing "SVN" rules.
If you are strongly against one of the rules, please shout now.
If you think I forgot something important, feel free to mention it.

I think the two major changes are the 4 space indentation and the "curly braces on a new line" which we want to put into place asap.

TEST YOUR CHANGES
1 - BEFORE committing a change to the SVN, run the Test Suite. If you don't know how to run the Test suite, ask me. It requires either Linux or Windows.
- the results of the tests are in Res/test/results.html
- If a test in the test suite fails, try to run the test suite a second time. There's a race condition that makes some tests fail if the framerate slows down. If a test fails twice, there's a high chance it's a real bug.
2 - If you didn't follow rule number 1, and your change breaks a test, be ready to hear me shout and scream.
3 - If you add a new card that you are not confident will work, test it. NO MORE "not tested, but I guess it should work" messages in the SVN, please.
4 - Don't commit half finished stuff in the SVN. The latest revision of the SVN should always be in a state good enough for it to be a "candidate for release". If you have huge changes in progress, make local backups on your hard drive, or create a branch in the SVN.

CODING STYLE
These rules are not "must" but "nice to have". People who don't respect them won't be shot in the head, people who respect them are cool guys.
  • We use the Unix encoding for carriage returns. If you use windows, there's probably an option for that in your editor. Visual Studio will usually ask you when there's a strange file. Always reply "Unix"
  • 4 space identation. Don't use tabs for indentation, use spaces. One tab = 4 spaces. There is a setting for this in Visual Studio.
  • the "=" sign should be separated by spaces. "a = 1;" is good. "a=1;" is not. "a= 1;" and "a =1;" are even worse.
  • curly braces go on a new line

    Code: Select all

    if (foo)
    {
        bar();
        stuff();
    }
    
  • If a scope contains only one line, it is ok to ommit the curly braces. It is fine to have them too. The following are all ok, use your judgment here:

    Code: Select all

    if (foo)
    {
        bar();
    }
    

    Code: Select all

    if (foo)
        bar();
    

    Code: Select all

    if (foo) bar();
    
  • parameters spacing: no space between function name and parenthesis, space after a comma separating parameters:

    Code: Select all

    int foo(int a, int b)
    this.foo(a, b);
  • Avoid to nest if/while/for too much. As a rule of thumb, If you have more than 3 "nested loops" levels, there is probably a better way
  • As a consequence of the rule above, we suggest to return early:

    Code: Select all

    if (not_applicable) return; // And don't care any more
  • in header (.h) files, use the following order: private, protected, public > Attributes then methods, group them by "goal/semantics" rather than alphabetically. No specific rule for .cpp files

    Code: Select all

    MyClass
    {
        protected:
            int mHeight;
            int mWidth;
    
            float stuff;
    
            int foo();
    
        public:
            int mPublicVar;
    
            MyClass(int stuff);
    };
    
  • Variable naming: we use camelCase and a simple prefix for global variables and attributes: gGlobal, mAttribute, noSpecificRuleForLocalVars.If possible we want to avoid global variables. First letter is lowercase.
  • Function and class naming: we use CamelCase, the first letter is uppercase.
  • no magic numbers, use enum or define instead:

    Code: Select all

    if (Constants::MAGIC_NUMBER == myVariable)
    is good,

    Code: Select all

    if (55 == myVariable)
    is not.
  • We love comments in the code, as long as they don't state the obvious
    we don't need

    Code: Select all

    int getHeight(); //gets the Height
    we love

    Code: Select all

    superHack(); // There's a bug with some cards that require to call this hack, see http://code.google.com/p/wagic/issues/detail?id=123
    

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Sun Nov 14, 2010 3:23 am

Anybody know of a free/cheap plug-in for vs2008/2010 that does code formatting? Eclispe comes with one out of the box. I don't want to have to use eclispe to reformat code. So anything is better than nothing(which is what vs currently does)


Mike

Btw I'll try and get at the very least all my code reformatted this weekend. So if you see a big check in by me, it's just reformatting and not any code changes.

MootPoint
Posts: 58
Joined: Fri Sep 24, 2010 7:44 am

Re: string parsing crashes

Post by MootPoint » Sun Nov 14, 2010 3:38 am

Excellent. Big thumbs up.

The one other coding style nuance around camelCase I'd like to make explicit, as I see some code with functions declared lowercase, others uppercase, and I'd like to make it consistent:

Function names and class declarations should be capitalized, whereas objects/variables shouldn't.

Code: Select all

class Foo
{
public:
void SomeFunction();
int mSomeVariable;
};

Foo foo;  // I know right away based on the capitalization that Foo is the class, foo is a local variable

wololo
Site Admin
Posts: 3727
Joined: Wed Oct 15, 2008 12:42 am
Location: Japan

Re: string parsing crashes

Post by wololo » Sun Nov 14, 2010 8:59 am

Ok, I like this rule, adding it now

mnguyen
Posts: 229
Joined: Thu Apr 29, 2010 4:13 pm

Re: string parsing crashes

Post by mnguyen » Sun Nov 14, 2010 9:25 am

+1 on the guidelines!

I would suggest that we break up long conditional statements into multiple lines and groups. This helps to visually differentiate between what is related and what isn't and helps minimize the amount of horizontal scrolling.

for instance, instead of writing
(Note that this is one line)

Code: Select all

if (_card == source && (!tap || !source->isTapped())  && game->currentlyActing()->game->inPlay->hasCard(source) && (source->hasType(Subtypes::TYPE_LAND) || !tap || !source->hasSummoningSickness()) ){
		
we could write

Code: Select all

if ( _card == source && 
     (!tap || !source->isTapped())  && 
     game->currentlyActing()->game->inPlay->hasCard(source) && 
     (source->hasType(Subtypes::TYPE_LAND) || !tap || !source- >hasSummoningSickness()) )
{
		
or something like it. It makes reading the code a little easier as it breaks up the conditional into modular parts as well as reduces the need to horizontally scroll
which is something most UI experts will say is bad if you already have vertical scrolling.

One other point that may be added is to have one statement per line.

This is a very short example, but I find this kind of coding very hard to read when stepping through lots of code.

Code: Select all

<statement>
<statement>
if (old != active) { if (old) old->zoom = kZoom_none; if (active) active->zoom = kZoom_level1; }
<another statement>
<another statement>
instead rewrite it to look like this:

Code: Select all

if (old != active) 
{ 
     if (old) 
         old->zoom = kZoom_none; 
     if (active) 
         active->zoom = kZoom_level1; 
}
i know there is an argument for collapsing the if statements, but I'm just writing this for illustration. I suppose with the new guideline of putting the brace on a new line will alleviate this particular situation. but it does not cover cases like this:

Code: Select all

            if ((*it)->show)
              {
                (*it)->y = 210;
                (*it)->zoom = kZoom_level2; (*it)->t = 0;
                if (!active) active = *it;
              }
Again, something very simple but imagine the middle statement having more than just two operations that span across the entire page.

That's all for now.

Thanks for listening,
Mike

Jean
Posts: 32
Joined: Tue Nov 18, 2008 5:12 am

Re: string parsing crashes

Post by Jean » Mon Nov 15, 2010 4:29 pm

Rules sound good to me.

Constants ? I think ALL_CAPS_AND_UNDERSCORES is the preferred way in most languages, but we also have people who use the kAppleNotation. Any opinion ?

Post Reply