Jump to content

  • Log In with Google      Sign In   
  • Create Account


Code Review Comments

  • You cannot reply to this topic
17 replies to this topic

#1 Steno   Members   -  Reputation: 115

Like
0Likes
Like

Posted 25 February 2013 - 10:38 PM

We have a process where every commit gets code reviewed and graded.  I've had some good ones come my way and these were all rated as 'F' in the American grading scale.  These are all real, so I hope someone gets a chuckle out of this.  Keep in mind that we are a purely C++ shop.

 

   void SetX(int iNewX) { iX = iNewX; }

 

Comment:  int iNewX needs to be const.

 

float fMinDistance = 1.0e10f;  // Some arbitrary large distance

 

Comment:  This value is too large for a float.  Use a double.

 

char szFilename[STR_LENGTH];

 

Comment:  szFilename needs to be szFileName!!!!!  (note: !!!!! is not exaggerated)

 

 

glPushMatrix();
...
glPopMatrix();

 

Comment:  I don't know anything about OpenGL, but even I can see this is stupid!

 

// Foo is a POD type
struct Foo {
  Foo() : x(0), y(0) {}
  ...

 

Comment:  POD types can't be structs.

Comment:  What does this even mean?

 

 

OK, I have paraphrased some of the comments, but they are real.  Feel free to troll this thread for your own code review horrors.


Edited by Steno, 25 February 2013 - 10:51 PM.


Sponsor:

#2 Alpha_ProgDes   Crossbones+   -  Reputation: 4688

Like
1Likes
Like

Posted 25 February 2013 - 10:43 PM

I'm not sure which fails worse. The code or the comments......


Beginner in Game Development? Read here.
 
Super Mario Bros clone tutorial written in XNA 4.0 [MonoGame, ANX, and MonoXNA] by Scott Haley
 
If you have found any of the posts helpful, please show your appreciation by clicking the up arrow on those posts Posted Image
 
Spoiler

#3 Steno   Members   -  Reputation: 115

Like
0Likes
Like

Posted 25 February 2013 - 10:55 PM

I'm not sure which fails worse. The code or the comments......

 

As far as code goes, in case your referring to the char* szFilename part, I fixed that.



#4 Bacterius   Crossbones+   -  Reputation: 8532

Like
0Likes
Like

Posted 26 February 2013 - 07:25 AM

Well I usually define floating-point infinity as 1e20 in prototype code if the actual value is too difficult to reach (e.g. no INFINITY macro, and you have to type a complicated templated expression that you usually get wrong the first time.. thank you type abstraction!). 1e10 is a bit low though.. depending on uses.


The slowsort algorithm is a perfect illustration of the multiply and surrender paradigm, which is perhaps the single most important paradigm in the development of reluctant algorithms. The basic multiply and surrender strategy consists in replacing the problem at hand by two or more subproblems, each slightly simpler than the original, and continue multiplying subproblems and subsubproblems recursively in this fashion as long as possible. At some point the subproblems will all become so simple that their solution can no longer be postponed, and we will have to surrender. Experience shows that, in most cases, by the time this point is reached the total work will be substantially higher than what could have been wasted by a more direct approach.

 

- Pessimal Algorithms and Simplexity Analysis


#5 Sik_the_hedgehog   Crossbones+   -  Reputation: 1608

Like
0Likes
Like

Posted 26 February 2013 - 07:51 AM

Doesn't (1.0f/0.0f) do the trick?


Don't pay much attention to "the hedgehog" in my nick, it's just because "Sik" was already taken =/ By the way, Sik is pronounced like seek, not like sick.

#6 swiftcoder   Senior Moderators   -  Reputation: 9860

Like
0Likes
Like

Posted 26 February 2013 - 08:34 AM

Doesn't (1.0f/0.0f) do the trick?

Not if floating point exceptions are enabled...

Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#7 Mussi   Crossbones+   -  Reputation: 1764

Like
0Likes
Like

Posted 26 February 2013 - 08:45 AM

Comment: szFilename needs to be szFileName!!!!! (note: !!!!! is not exaggerated)

Filename can be a single world, both variants are widely used.

Comment: I don't know anything about OpenGL, but even I can see this is stupid!

Based on those two lines?

// Foo is a POD type
struct Foo {
  Foo() : x(0), y(0) {}
  ...
Comment: POD types can't be structs.
Comment: What does this even mean?

Foo is not a POD type, but not for the reasons you mentioned.



#8 swiftcoder   Senior Moderators   -  Reputation: 9860

Like
0Likes
Like

Posted 26 February 2013 - 08:49 AM

Foo is not a POD type, but not for the reasons you mentioned.

Huh? It has neither a user-defined copy constructor nor destructor.

A default constructor does not prevent a class/struct from being a POD type.

Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#9 Mussi   Crossbones+   -  Reputation: 1764

Like
0Likes
Like

Posted 26 February 2013 - 09:05 AM

A default constructor does not prevent a class/struct from being a POD type.

Frankly I'm not sure anymore. Quoting from wikipedia: 'Moreover, a POD class must be an aggregate, meaning it has no user-declared constructors', I'm not sure if declaring a defualt constructor falls under user-declared constructors.



#10 swiftcoder   Senior Moderators   -  Reputation: 9860

Like
0Likes
Like

Posted 26 February 2013 - 09:53 AM

Frankly I'm not sure anymore. Quoting from wikipedia: 'Moreover, a POD class must be an aggregate, meaning it has no user-declared constructors', I'm not sure if declaring a defualt constructor falls under user-declared constructors.

Hmm. Now I'm confused too. Did this behaviour change in C++11?
struct MyPOD {
	int x, y;

	MyPOD() : x(0), y(0) {}
};

union MyUnion {
	int a;
	MyPOD pod;
};

int main() {
}
Clang with -std=c++11 compiles this just fine, but without -std=c++11 it gives me an error.

Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#11 Mussi   Crossbones+   -  Reputation: 1764

Like
1Likes
Like

Posted 26 February 2013 - 09:57 AM

Hmm. Now I'm confused too. Did this behaviour change in C++11?

Seems so: http://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-how-why-are-they-special/7189821#7189821

 

Edit: or well I'm not sure actually :P, any experts on this?


Edited by Mussi, 26 February 2013 - 09:59 AM.


#12 Cornstalks   Crossbones+   -  Reputation: 6974

Like
2Likes
Like

Posted 26 February 2013 - 10:18 AM

WTF? It just ate the important parts of my post... editing to try and add it back in...

What I tried to write:

Not an expert, but I was just writing a response about this...
 
Consider the following:

#include <type_traits>
#include <iostream>
 
struct Foo {
  Foo() : x(0), y(0) {}
  int x;
  int y;
};
 
struct Bar {
  Bar() = default;
  int x;
  int y;
};
 
int main()
{
    if (std::is_pod<Foo>::value) std::cout << "Foo is a POD" << std::endl;
    else                         std::cout << "Foo is *not* a POD" << std::endl;
    
    if (std::is_pod<Bar>::value) std::cout << "Bar is a POD" << std::endl;
    else                         std::cout << "Bar is *not* a POD" << std::endl;
}

This outputs:

Foo is *not* a POD
Bar is a POD
 

 

Let's look at what's going on...

 

Section 12.1, paragraph 5
A default constructor for a class X is a constructor of class X that can be called without an argument. If
there is no user-declared constructor for class X, a constructor having no parameters is implicitly declared
as defaulted (8.4). An implicitly-declared default constructor is an inline public member of its class.

...

A default constructor is trivial if it is neither user-provided nor deleted and ...

(note the last line, so Foo's constructor is not trivial because it is user-provided)

So let's look at what a POD is:

Section 9, paragraph 10
A POD struct is a class that is both a trivial class and a standard-layout class, and has no non-static
data members of type non-POD struct, non-POD union (or array of such types).

 

So now we ask, is Foo a trivial class?

Section 9, paragraph 6
A trivial class is a class that has a trivial default constructor (12.1) and is trivially copyable.

Nope, looks like Foo is not a trivial class, because it does not have a trivial default constructor, therefore it cannot be a POD.

Note that this is for C++11. I'd have to read through C++03 more thoroughly to be able to comment on C++03.


Edited by Cornstalks, 26 February 2013 - 10:28 AM.

[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#13 Olof Hedman   Crossbones+   -  Reputation: 2752

Like
0Likes
Like

Posted 26 February 2013 - 10:33 AM

The change in C++11 is to unions.

From the wikipedia page: "Unions can now contain objects that have a non-trivial constructor." and "If so, the implicit default constructor of the union is deleted, forcing a manual definition."

So, its ok to have non-POD classes in unions now.
Though, it does say you have to add a default constructor to the union if you do... so still something unexplained going on with the union...

Edited by Olof Hedman, 26 February 2013 - 10:35 AM.


#14 Cornstalks   Crossbones+   -  Reputation: 6974

Like
0Likes
Like

Posted 26 February 2013 - 10:42 AM

The change in C++11 is to unions.

There was certainly a change to the definition of PODs


[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#15 swiftcoder   Senior Moderators   -  Reputation: 9860

Like
0Likes
Like

Posted 26 February 2013 - 10:45 AM

Though, it does say you have to add a default constructor to the union if you do... so still something unexplained going on with the union...

Ok, this was bugging me, but I think it makes sense.

You can have as many user-defined constructors as you like, but you have to have a default constructor, so that the compiler can construct an instance when it needs to.

Tristam MacDonald - Software Engineer @Amazon - [swiftcoding]


#16 Olof Hedman   Crossbones+   -  Reputation: 2752

Like
0Likes
Like

Posted 26 February 2013 - 11:02 AM


The change in C++11 is to unions.

There was certainly a change to the definition of PODs


Well, my point was that the reason that swiftcoders code compiled wasn't because the struct was POD (it wasn't), but because unions now allow non-POD members.

#17 Cornstalks   Crossbones+   -  Reputation: 6974

Like
0Likes
Like

Posted 26 February 2013 - 11:13 AM

 

The change in C++11 is to unions.

There was certainly a change to the definition of PODs

 

Well, my point was that the reason that swiftcoders code compiled wasn't because the struct was POD (it wasn't), but because unions now allow non-POD members.

Ah, that makes sense now. I didn't realize it was directed at his post.


[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#18 Alpha_ProgDes   Crossbones+   -  Reputation: 4688

Like
0Likes
Like

Posted 01 March 2013 - 03:35 AM

Wow. Maybe you should change your codebase over to D. Because if 3 C++ experts (or advanced users) are all confused about what is standard, then debug hell must have more than 9 circles.


Beginner in Game Development? Read here.
 
Super Mario Bros clone tutorial written in XNA 4.0 [MonoGame, ANX, and MonoXNA] by Scott Haley
 
If you have found any of the posts helpful, please show your appreciation by clicking the up arrow on those posts Posted Image
 
Spoiler





PARTNERS