Sign in to follow this  
slayemin

Peer-Code reviews?

Recommended Posts

Is there a good place to submit code for appreciation and peer reviews? :) I'm trying to improve my code with standard best practices, and I think it would be beneficial to have other people look at my code to see what they like/dislike and for me to look at code writen by others to see what they do that makes it good or bad. I guess the first question to ask is "What are the criteria for judging whether code is good/bad?" (besides if it works) Here's my personal criteria: [abstract] -Code is well commented so that anyone can figure out whats going on, and picking up the project 6 months later is easy -Variable names are well thought and descriptive of the data they represent -The code is formatted and easily readable (white space, tabs/spaces) -The code is flexible and maintainable and broken down into managable sections

Share this post


Link to post
Share on other sites
Quote:
Original post by slayemin
Is there a good place to submit code for appreciation and peer reviews? :)
I'm trying to improve my code with standard best practices, and I think it would be beneficial to have other people look at my code to see what they like/dislike and for me to look at code writen by others to see what they do that makes it good or bad.

I guess the first question to ask is "What are the criteria for judging whether code is good/bad?" (besides if it works)

Here's my personal criteria:
[abstract]
-Code is well commented so that anyone can figure out whats going on, and picking up the project 6 months later is easy
-Variable names are well thought and descriptive of the data they represent
-The code is formatted and easily readable (white space, tabs/spaces)
-The code is flexible and maintainable and broken down into managable sections



Well a lot of it may depend on whether you are writing all of the code yourself or you are working in a team environment. Begin the only person touching the code it's usually easier to create some form of coding style guide. In team environments you may end up with a few luke warm arguments concerning certain topics.

With code reviews you want to have a list of things to check for. Even if it sounds obvious it's still beneficial to have it written down. There are quite a few things that you might consider:


1. Checking to make sure that local variables are properly initialized.
2. At every return statement make sure that allocated memory is released (if no longer needed).
3. Checking for NULL pointers.
4. If your function throws an exception check to make sure all code that calls the function "catches" the exceptions thrown.
5. As with #4 if the function returns an error condition make sure it's checked.
6. If the code is in C++ always check any call to virtual functions from within the destructor. C++ has scalar destructors so the actual behavior is not always what the programmer intended.
7. Check function parameters and class methods to determine if the use (or omission) of 'const' modifiers apply. Using 'const' in the proper place can help detect undesirable actions during compile time. (i.e. trying to modify or delete an object when it shouldn't).
8. ---snipped---
9. Make sure all class attributes are initialized when an object is instantiated.

10. Make sure all loop and conditional statements have braces. Even if the statement is a single line:

while(i < 5) i++;

becomes:
while(i < 5) {
i++;
}


(Brace placement style is really up to you)

11. Make sure destructors delete (if necessary) all memory the object has allocated. Of course if it's still in use don't delete it :D



A lot of this depends on the language you are using but this should get you started. There will always be tradeoffs in cases where you may find items that eat up way too much cpu time. You just have to decide which tradeoffs to make.

Also it's usually a good idea to make your code as consistent as possible regardless of build mode (debug or release). If the error/failure/bug can happen in debug it can happen in release - checking it only in debug may not be the best solution. With the exception of say logging and the dynamic_cast operator I mentioned above you'll need to decide which pieces of code you want to be consistent across all builds.

[Edited by - Helter Skelter on June 29, 2005 8:25:47 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Helter Skelter
8. If for ANY reason you are using typecasting to convert object types triple check everything. A good approach is to enable RTTI for debug builds and use the following macro:

#define DYNAMIC_CAST(className, object) dynamic_cast<className>(object)

for release modes use:

#define DYNAMIC_CAST(className, object) ((className)object)

In the event you have an incorrect cast it will fail. In release mode RTTI is removed and the unsafe casts are used. This is a trade off in hope of finding incorrect casts in debug while retaining execution speed.


Are you serious?? Make your code less safe for release mode only??? for speed?? I've never seen RTTI show up on a profile and we use it pretty heavily. This seems like an optimization that should never be:)

what's the rational behind this??

Cheers
Chris

Share this post


Link to post
Share on other sites
I've found that typecasting is EVIL. :) What is RTTI?

I don't use the try{}...catch(exception){} mainly because I dont know what it really does or how it works.
I do like to use switch statements, since the default case is excellent for unexpected errors.

would you still try to deallocate memory if you are using C# .NET?

Share this post


Link to post
Share on other sites
Quote:
Original post by chollida1
Are you serious?? Make your code less safe for release mode only??? for speed?? I've never seen RTTI show up on a profile and we use it pretty heavily. This seems like an optimization that should never be:)


Oh Hells I'm not serious. Actually that section was copied from my one of my debug header files which obviously hasn't had the main comment updated in a while. Incidently the header originally written in 1997 for embedded development. At least it was rational at one point :D

Quote:
what's the rational behind this??


There is none unless you're targeting mobile devices.


At least I get a lot of code reuse :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Helter Skelter
Oh Hells I'm not serious. Actually that section was copied from my one of my debug header files which obviously hasn't had the main comment updated in a while.


Then why do you reproduce it here if you know it is incorrect?

Quote:
There is none unless you're targeting mobile devices.


Again, this makes your post rather misleading.

Quote:
Original post by slayemin
What is RTTI?


Run-time type information. A bit of data the compiler can use to keep track of the real type of an object. For example, if B derives from A, you can write A* ptr = new B; The static type of the object accessed from ptr will be A (because it is an A*), but its real type is B (because you did new B). The compiler automatically keeps track of that for polymorphic types (types with at least one virtual function), but it is generally possible (through a compiler option) to force it to generate it for all user-defined types. The C++ dynamic_cast checks that information to verify that you are casting legally - so dynamic_cast<B*>(ptr) will only work if ptr really points to a a B object (or derived from B), not if it points to an A object (via A* ptr = new A;).

Replacing a static_cast with a dynamic_cast for debugging purposes MAY help you catch some errors (casting to incorrect types), but there just is NO WAY that you can replace a (necessary) dynamic cast with a static cast "for the sake of speed". Which is where Helter Skelter's post becomes confusing, since it seems that's what he advocates.

Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
Then why do you reproduce it here if you know it is incorrect?


Because I mistakenly assumed the comments in that particular file had been updated so I just copied and pasted everything from "A good approach...." on. Unfortunately the only thing that was changed in the primary comments was the additions in the change log. Honest mistake. I corrected it. Let's dwell on it for a few more hours.

Quote:
Quote:
There is none unless you're targeting mobile devices.


Again, this makes your post rather misleading.


Perhaps. It might be quite relevant to remove usage of RTTI and dynamic casts when targeting mobile or embedded devices where CPU usage and memory footprint are a consideration. When the file was created I ws developing for embedded systems with limited resources. Removal of RTTI and dynamic_cast was essential.

Share this post


Link to post
Share on other sites
Quote:
Original post by Helter Skelter
Honest mistake. I corrected it.


Fair enough.

Quote:
Perhaps. It might be quite relevant to remove usage of RTTI and dynamic casts when targeting mobile or embedded devices where CPU usage and memory footprint are a consideration.


The question is - what are you replacing the functionality with? You say you remove the cast for performance reasons without addressing the fact that it makes the program plainly incorrect. Either the dynamic_cast was unnecessary because you knew the cast really was safe and you could use a static_cast in the first place, or it was necessary because you're not sure of the dynamic type of the object and switching to a static_cast (or to a C cast) is simply incorrect.

Quote:
When the file was created I ws developing for embedded systems with limited resources. Removal of RTTI and dynamic_cast was essential.


It possible not to use the compiler's RTTI, but then the classes need to be designed rather differently than if you rely on it. It's not just a matter of macroing away the dynamic_cast. You generally end up adding a type field or some pointers which serve precisely the same role as the data the compiler would add if you had enabled RTTI.

Or you just plainly not use runtime information at all, in which case the dynamic_cast was pointless anyway.

Share this post


Link to post
Share on other sites
Quote:
Original post by Fruny
The question is - what are you replacing the functionality with? You say you remove the cast for performance reasons without addressing the fact that it makes the program plainly incorrect. Either the dynamic_cast was unnecessary because you knew the cast really was safe and you could use a static_cast in the first place, or it was necessary because you're not sure of the dynamic type of the object and switching to a static_cast (or to a C cast) is simply incorrect.



This might be easier with a little bit of background.....This is long and boring so feel free to skip to the last paragraph:


When the debug support libs were written the target platform was a cablebox with 2mb of RAM and a 20mhz 68000 compatible chip (68360 I believe - same on as the old CDi boxes). The project was for Disney who did the visual prototype on a high end SGI box. Disney does NOT do things small so you can imagine how well it translated from an SGI box to a STB.

With RTTI enabled, all of the type info and support libs ate up a bit over 300k. Some operations such as downloading program guide data ended up dealing with a lot of upcasting. Due to the tight memory constraints and performance issue it was decided to dump the use of RTTI except for debug mode only. This eventually expanded to support specific inhouse testing with RTTI enabled for release builds (see below).

The approach in using the cast macro allowed us to utilize RTTI in topossible bugs as possible during development and not exceed the memory footprint in release. In release mode the casts became traditional (unsafe) C style casts (or static_casts depending on file). Since the development boxes had 4mb of RAM it was possible to do release testing with RTTI enabled as an additional sanity check but not for final production builds. Was it the "right" decision? Given the lack of workable alternatives I definitely think it was. It did allow us to do a considerable amount of testing with RTTI even if it wouldn't be available in final production. There were several macro variations and some for static_cast as well.

This could have been easily avoided if they had gone with the 4mb version of the box at an additional ~$3 per unit. But with the initial rollout being 3 to 4 million STB's the extra $9 to $12 million changed the issue from sanity to politics.


Incidently the section of the comments I indended to paste included the various cast macros with options to substitute/remove casts (say switch dynamic_cast to static_cast or no cast at all) to help locate casts that are no longer needed. It doesn't however remove the casts during release builds. Since I haven't had to do a cast in a while today was the first time in ages that I've even looked at the file.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this