Jump to content
  • Advertisement
Sign in to follow this  
gretty

Provide a quick code-review of c++ code?

This topic is 733 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

Apologies if my post is not quite according to the standards of this sub-forum but.. Would anyone want to take the time to do a code-review of my c++ code. Not a standard time intensive review, just review the main/only header and cpp files and review format, c++11 standards, idioms and any big no-no's that stand out? The application is for my github portfolio.

 

The main application code is found in the Components folder. The bulk of the code resides in another repository (the CBA API) which please take a look at also although this hasn't been refactored in a while.

 

https://github.com/sazr/WindowTiler

Edited by gretty

Share this post


Link to post
Share on other sites
Advertisement

I had a quick browse through the App code without actually reading things, and noticed a total lack of comments for the methods. Reverse engineering what a function does, with its entry and exit values and conditions won't work over a longer period of time, or when you add more lines of code.

 

The other thing was your switch at line 489, https://github.com/sazr/WindowTiler/blob/master/Components/App.cpp#L489

Why is everything at the same indent and no empty lines? It's impossible to find your 'case' lines this way.

Share this post


Link to post
Share on other sites

I had a VERY brief look at App.

 HWND hwndTry, hwndWalk = NULL;

Are you expecting that to initialise both to NULL?

 

Alberth's right about that switch statement, it does look bad formatted like that. If I recall it is because you have brackets in each case which I don't think Visual Studio likes when it comes to auto formatting. You don't need the brackets there in your case but they aren't really hurting (apart from messing with the auto formatting).


A little further down you start switching to 'java style' brackets. E.g

if() {
   DoSomething();
}

instead of

if()
{
   DoSomething();
}

Which isn't exactly a problem to use but consistency goes a long way so stick to one style. I spotted a loop doing the same somewhere too. It started at line 567.

 

 

Share this post


Link to post
Share on other sites
I was reading Alberth's comments so I looked at the code starting at line 487:
	int yPos = 0;

	switch (barData.uEdge)
	{
	case ABE_LEFT:
	case ABE_RIGHT:
	{
		yPos = btnDim.top - lbH - 10;
	}
	break;
	case ABE_TOP:
	{
		yPos = btnDim.bottom + 10;
	}
	break;
	//case ABE_BOTTOM:
	default:
	{
		yPos = btnDim.top - lbH - 10;
	}
	break;
	}

How about this instead?
	int yPos = (barData.uEdge == ABE_TOP) ? btnDim.bottom + 10 : btnDim.top - lbH - 10;

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!