Jump to content
  • Advertisement
Sign in to follow this  
Steno

Code Review Comments

This topic is 2030 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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

Share this post


Link to post
Share on other sites
Advertisement

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!