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]

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 :)

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.

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.