• Advertisement

Archived

This topic is now archived and is closed to further replies.

Coding Standards in the Workplace

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

I know there is a decent amount of profesional programmers on these forums so I hope to get a few replies. How does your company handle coding practises in the workplace? Is there a certain way everyone''s code should look? Do they stress Hungarian notation? How about commenting styles? Also, how much does your workplace enforce them? Do you risk getting fired if you don''t comment a function?

Share this post


Link to post
Share on other sites
Advertisement
I''d call our standards semi-hungarian. They love the variable naming conventions but not some of the more weird stuff. They especially love using variable names that mean something, even if they are half a page long. Tabs are important, every job I''ve had requires tabs-as-spaces, with 2 as the indentation and tab value.

Commenting style is not a big deal, but comments themselves are encouraged at my current job. It''s not unusual to find "// touch and die, call me first at Ext ####" types of comments. At Raytheon they were extremely retentive about comment blocks before every single function and at the top of every single file with very stringent formatting requirements, but that''s because they were trying to get level 3 for ISO 9000, so we pretty much gritted our teeth and used the copy/paste hotkeys a lot.

Even at Raytheon you wouldn''t get fired for not commenting a function. You''ll likely get fussed at and told to do it, but not seriously yelled at. The companies I''ve worked at consider coders that are familiar with their (massive) projects to be pretty valuable and they won''t fire us over a few comments. Comment requirements are not even close to being as stringent as they are in a classroom setting, because they more or less expect that anyone reading the code can, well, read code, and unless you''re doing something weird or being bad about what you''re naming your variables you usually won''t need to comment too much.

-fel

Share this post


Link to post
Share on other sites
Actually, even if you''re doing something not-so-weird, it''s always good to add a little comment. Just tonight I came across some code in our 7 year old code base that called a function called "SaveTempGame()" or something similar. This function did just what it said - it saved a copy of the current game into a temp folder. What it did not bother to explain is WHY it did it. Nor did any place that called it. Nor, in fact, did anything anywhere in the codebase. So, while its effects are easy to figure out, its intent isn''t so easy.

And the topic of conventions - if you ever want to see hell, look at a code base that was built by over 40 engineers over 7 years across 5 platforms, most of whom did not have any formal training, and you will quickly understand why coding conventions in a multi-million line code base are very important for maintainability.

And of course, there is zero accountability. In fact, most of the people who wrote some of the older legacy functions aren''t even in the industry anymore, let alone available to explain what the heck they meant by a piece of code - and even if they are, probably couldn''t. It was for this very reason I made a comment to a coworker tonight (one of many 16+ hour days) along the lines of, "The only reason we''re still here tonight is because of the sloppiness / laziness / inability of the people before us."

I also coined the terms "ungineer" and "congrammer".

Ah well. 1am and the build''s almost done.
-scott

Share this post


Link to post
Share on other sites
My last job had a style guide that they expected us to use. It was essentially Hungarian, with a few minor differences. There was no real mandate when it came to commenting style, but they were encouraged. Like fel, I was expected to set VC++ to convert tabs to spaces and use a tab/indent size of 2, so that the code would line up properly for people using other IDEs (such as JOVE, Microsoft C, or Borland C). I don't think that failing to adhere to the standard is something they'd fire you over.

At my current job, if there's a coding standard, I've never heard about it. Looking at our code base, if there is one, it must change every few months

Share this post


Link to post
Share on other sites
We don''t really have an official coding standard because we use so many different languages. My company develops software to manage servers / routers. It collects the information from an agent and presents it on a console. Our agent runs on well over 200 OS / architecture combos, and has about 100 major components that you can plug into said agent and console framework.

There are internal developer websites that offer suggestions on how to format your code, but there''s no single standard or rule and the suggestions aren''t available for all of the languages we use. The format differs between platform and development group by quite a bit.

Share this post


Link to post
Share on other sites
Our company has no formal rules, except every piece of code must be scrutinised and approved by an independent reviewer (someone else in the company) before it can be used in a final product. This is an excellent encouragement to make code clear and friendly, ''cos no one wants their code to fail the review.

Share this post


Link to post
Share on other sites
My last job had no coding standards whatsoever. I spent a lot of time converting other people''s code - not because I preferred my standard to theirs - but because I preferred my standard to no standard at all (eg. arbitrary indenting for one).

[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites
We have fairly strict coding standars, based on the Ellemtel Rules and Recommendations. Formatting, commenting, naming, and language usage are all expected to conform to the standard. Failing to do so won''t get you fired, but it will make coworkers and supervisors rather unhappy.

Share this post


Link to post
Share on other sites
A big company I was working at before I moved here had some pretty decent code standards. The software was so immense there had to be some. Stuff like prefixes for classes, good variable names with correct spellings (no "nbrOfPpl"). Pretty much just followed what Code Complete talked about. We had to do regular code reviews in which fellow employees/friends/enemies got to make fun of your code while it was shown on the wall. When people could not figure out what was going on at first glance, it razed havoc, thus making all of us make really nice readable code. Think your code is well written and readable? Then you have either had a similar experience, or living in a fantasy land.

Through my contract jobs, I have never had a standard forced on me, and just kept the one from the last job. I think it is important to keep to the same standard at least in your own code if there is not a company policy. Nothing like having code where the first letter of variable is lowercase for some members, and upper for the rest.

Share this post


Link to post
Share on other sites
Where I work, all the source code is available to the customer (as they may want to maintain it after we have left). So Our comments and naming conventions are customer specific.

As an aside to this, I thought that it would make people more likely to play but we work in a rapidly changing field and if they tinker they lose their warranties so the worst we get is we think theres a bug here, and an explanation of the code which actually helps a great deal. (Doesn''t stop us having to fix it though)

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
How many comments are you generally encouraged to do (e.g. how many lines can you go without comments roughly)?

I normally keep my functions short (no more than about 100 lines) but always document what they do at the top. I might comment the more obscure lines but this very rare (about once every 200-300 lines) because descriptive variable names and short functions mean I don''t have to. Is this exceptable in a job?

Share this post


Link to post
Share on other sites
Unfortunately, my company has no standards. Actually, we just implemented a standard tab size . I personally use a simplified hungarian notation, with long descriptive variable names. I try to follow good programming practice (ie encapsulation) whenever posible. I comment librally. Even when programming on little projects, the extra effort more than pays for itself with saved time later on (I can''t remember what was going through my head 2 days ago, so it really helps).

I''ve tried to get a standard going at our company a few times. The reply I get is either laughter, or silence. Oh well. Our team has only 7 programmers, and they''ve been here for the entire life of the company, so it''s not that bad (yet).

Share this post


Link to post
Share on other sites
quote:
Original post by Myopic Rhino
At my current job, if there''s a coding standard, I''ve never heard about it. Looking at our code base, if there is one, it must change every few months =)


Story of my life. Where I work I am one of 2 programmers: the other guy does mostly C while I code Java, each of us getting drug into a little ASP or PHP every so often. So basicly, my Java coding style is the company''s standard.

I''m sure it''s different in bigger companies, but in little ones you get a lot of freedom on how you structure your projects. This can be good in some cases, and very bad in others. Good because I get the freedom to code as I whsh, bad because when less diciplined coders than I leave they tend to leave 6000 line VB COM objects laying around without a single comment or readme.



"There is no reason good should not triumph at least as often as evil. The triumph of anything is a matter of organization. If there are such things as angels, I hope that they''re organized along the lines of the mafia." -Kurt Vonnegut

Share this post


Link to post
Share on other sites
quote:
Original post by Anonymous Poster
How many comments are you generally encouraged to do (e.g. how many lines can you go without comments roughly)?

I normally keep my functions short (no more than about 100 lines) but always document what they do at the top. I might comment the more obscure lines but this very rare (about once every 200-300 lines) because descriptive variable names and short functions mean I don''t have to. Is this exceptable in a job?

Generally, that''s not acceptable. We have required per-function comment blocks that precede each function, but every block of code is commented. "Self-documenting code" is what you''re describing, and that''s pretty much an urban programmer''s legend IMHO; it might seem self-documenting to you, but probably isn''t to someone else.

For example, here''s some "appropriately-commented" (*cough*, I hope) code that serializes resources for one of our projects:
  
// serialize resources into a data stream

void ResourceMakerManager::
makeStream (RawData &stream,
const ResourceInfoGroup &resources)
{
// first field is the size of objects

const int numResources = resources.size ();
FieldIo::writeField (stream, &numResources, sizeof (numResources));

// append each resource to the stream


// need to do two-step loop, once to reassign IDs, next to generate

// (animation frames need to know their animation''s id)

ResourceInfoGroup::const_iterator resIt = resources.begin ();
const ResourceInfoGroup::const_iterator resEnd = resources.end ();
unsigned int newID = 1; // renumber IDs as we go

map<unsigned long, unsigned long> idMap; // old id to new id

for ( ; resIt != resEnd; ++resIt)
{
const unsigned long id = resIt->first;
assert (idMap.find (id) == idMap.end ()); // unique id

idMap[id] = newID++;
}

// now generate, passing the map in to each

for (resIt = resources.begin (); resIt != resEnd; ++resIt)
{
ResourceInfo *pRes = resIt->second;
pRes->generate (stream, idMap);
}
}

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
quote:
Original post by Stoffel
[quote]Original post by Anonymous Poster
How many comments are you generally encouraged to do (e.g. how many lines can you go without comments roughly)?

I normally keep my functions short (no more than about 100 lines) but always document what they do at the top. I might comment the more obscure lines but this very rare (about once every 200-300 lines) because descriptive variable names and short functions mean I don''t have to. Is this exceptable in a job?

Generally, that''s not acceptable. We have required per-function comment blocks that precede each function, but every block of code is commented. "Self-documenting code" is what you''re describing, and that''s pretty much an urban programmer''s legend IMHO; it might seem self-documenting to you, but probably isn''t to someone else.

For example, here''s some "appropriately-commented" (*cough*, I hope) code that serializes resources for one of our projects:
    
// serialize resources into a data stream

void ResourceMakerManager::
makeStream (RawData &stream,
const ResourceInfoGroup &resources)
{
// first field is the size of objects

const int numResources = resources.size ();
FieldIo::writeField (stream, &numResources, sizeof (numResources));

// append each resource to the stream


// need to do two-step loop, once to reassign IDs, next to generate

// (animation frames need to know their animation''s id)

ResourceInfoGroup::const_iterator resIt = resources.begin ();
const ResourceInfoGroup::const_iterator resEnd = resources.end ();
unsigned int newID = 1; // renumber IDs as we go

map<unsigned long, unsigned long> idMap; // old id to new id

for ( ; resIt != resEnd; ++resIt)
{
const unsigned long id = resIt->first;
assert (idMap.find (id) == idMap.end ()); // unique id

idMap[id] = newID++;
}

// now generate, passing the map in to each

for (resIt = resources.begin (); resIt != resEnd; ++resIt)
{
ResourceInfo *pRes = resIt->second;
pRes->generate (stream, idMap);
}
}



The reason I bring this up is because I''m looking for a programming job and have be polishing up some old projects. Initially, I started commenting almost every line. I then looked over it and found out the majority of comments were completely redudent. In that I mean they didn''t add more understanding and they simply required too much maintainence if you changed small details.

Please note by the way, I am not against comments. I would happily comment code if I was told to do so and I document my functions very well. To get an idea what I mean, I''ll use your code example.

I would normally comment what the parameters are, but they seem obvious in this example. I would remove the first comment because it is obvious what the first two lines do (I would also just move the variable contents directly into the writeField function call).

I would rename idMap to oldIdToNewId and remove the comment. The other comments I would leave though. I would possibly move the two for loops into seperate functions and then just document what those functions do.

Would you find that acceptable? If I go to a job interview, I would have to defend myself on too many comments or too few comments I don''t find a good balance

Share this post


Link to post
Share on other sites
quote:
Original post by Anonymous Poster
The reason I bring this up is because I''m looking for a programming job and have be polishing up some old projects. Initially, I started commenting almost every line. I then looked over it and found out the majority of comments were completely redudent. In that I mean they didn''t add more understanding and they simply required too much maintainence if you changed small details.

In that case, you''re doing the wrong kind of comments. Comments should be independent of implementation details. They are the bridge between the design stage and the actual implementation. Most comments should stay the same when the code changes, as they are referring to the purpose of the code, not the composition of the code. Other comments might be noting where code has to be added or taken out, and these are easily amended when necessary.

I heavily comment my code, and have never found comments to get out of date. It''s a phenomenon that others talk of, but which I have never witnessed in my own code. Yet others say that my code is very well commented. So it''s obviously down to how you choose to comment, rather than how much you do so.

[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
quote:
Original post by Kylotan
[quote]Original post by Anonymous Poster
The reason I bring this up is because I''m looking for a programming job and have be polishing up some old projects. Initially, I started commenting almost every line. I then looked over it and found out the majority of comments were completely redudent. In that I mean they didn''t add more understanding and they simply required too much maintainence if you changed small details.

In that case, you''re doing the wrong kind of comments. Comments should be independent of implementation details. They are the bridge between the design stage and the actual implementation. Most comments should stay the same when the code changes, as they are referring to the purpose of the code, not the composition of the code. Other comments might be noting where code has to be added or taken out, and these are easily amended when necessary.

I heavily comment my code, and have never found comments to get out of date. It''s a phenomenon that others talk of, but which I have never witnessed in my own code. Yet others say that my code is very well commented. So it''s obviously down to how you choose to comment, rather than how much you do so.



Could you please give some example code which demonstrates what you think is an acceptable level of commenting? I understand and agree with what you are saying but it is still very hard to see how often you comment.

I keep all my varible and function names descriptive. I comment what functions do, their parameters and what they return. I also comment other important things like what member variables are for (they have a larger scope and hence need to be documented more). My functions are kept as short as I can so the function description is normally enough for you to understand what it is doing. If it is obvious what code does, commenting it is simply just adds to maintainence. If a piece of code is hard to understand, which is quite rare, I will comment it.

Short contrived example (maybe too simple) of how I would do it:


// Updates the player. The player may move left or right if they are alive.
// The player will die if their health is below zero.
//
// Parameter deltaTime - timestep in seconds update advances by.
void CPlayer::update(int deltaTime)
{
if (health < 0)
{
die();
}
else
{
if (keyPressed(leftKey))
{
moveLeft(deltaTime);
}
else if (keyPressed(rightKey))
{
moveRight(deltaTime);
}
}
}


It is very easy to understand what is going on because of the naming and the function documentation. The function body does not need comments because it is so obvious what it does.

Example of what I think is excessive commenting:


// Updates the player. The player may move left or right if they are alive.
// The player will die if their health is below zero.
//
// Parameter deltaTime - timestep in seconds update advances by.
void CPlayer::update(int deltaTime)
{
// If health goes below zero
if (health < 0)
{
// Kill player
die();
}
else // Only update movement if player didn''t die
{
// If left key is pressed
if (keyPressed(leftKey))
{
// Move player left for update time
moveLeft(deltaTime);
}
else if (keyPressed(rightKey)) // If right key pressed
{
// Move player right for update time
moveRight(deltaTime);
}
}
}


No extra understanding is added. I would actually say it is less readable now.

By the way, I disagree with you about comments never going out of date. Function documentation shouldn''t be changing much, so they are fine, but comments in the latter example are just a hassle to keep up to date. Even if you do not find this, I would find this with excessively commented code and commercial code is written for many different people to maintain. If, for example, you are just doing some small alterations here and there, like fixing bugs, refactoring and adding new features, it is very easy to forget to alter comments if they are present on most lines. You must be very alter if this has never happened to you.

For example:


// If health goes below zero
if (health < 0)
{
// Kill player
die();
}
else if (stunned) // If player stunned
{
// Play sound of player being stunned
playSound(stunnedSound);
}
else // Only update movement if player didn''t die
{
// If left key is pressed
if (keyPressed(leftKey))
{
// Move player left for update time
moveLeft(deltaTime);
}
else if (keyPressed(rightKey)) // If right key pressed
{
// Move player right for update time
moveRight(deltaTime);
}
}


Here I have made the player play a sound and stop them moving when they are stunned. The comment "// Only update movement if player didn''t die" is now out of date because I just forgot to check it was still accurate. This is a very simple example and in real code it is much easier to forget these things. Even if I skimmed over the comments I might still miss the inaccuracy because it sounds about right so I''d have to be on my toes to stop it. I don''t know that much about it but the eXtreme Programming (XP) coding practice has reasons as well why you shouldn''t comment as much as this and write self documenting code instead.

I liked to say again that I am not against comments; code should be well documented. I just dislike comments that do not add anything and are simply a waste of time to type and maintain.

Share this post


Link to post
Share on other sites
quote:
Original post by Anonymous Poster
// Updates the player. The player may move left or right if they are alive.
// The player will die if their health is below zero.


See, I consider these to be bad comments. If you add stuff to or remove stuff from the update routine, you now have to change the comment. I comment functions with a conceptual overview of what the function does - usually a one sentence description such as "//checks for status changes such as death, and processes movement based on keypresses". That tells you most of the same information and is less likely to become inaccurate.

Also, having big comment blocks at the top of a function is often a bad idea. When you alter line 50 of a function, you would then have to scroll up a page or two in order to find the comment that you might have to check. The documentation for a function should rarely be explaining everything that the function does - rather, it should explain the circumstances under which you would call it, and what the consequences are. I rarely find this takes more than a sentence. And with a well-designed system, this rarely changes.

Your example of excessive commenting is indeed excessive.

quote:
By the way, I disagree with you about comments never going out of date. Function documentation shouldn''t be changing much, so they are fine, but comments in the latter example are just a hassle to keep up to date.

Yeah, but now you''re saying "bad comments are a hassle to keep up to date". I say that, "good comments require next to no work to keep up to date". They usually impart information about the system at a high enough level that they are constant.

quote:
If, for example, you are just doing some small alterations here and there, like fixing bugs, refactoring and adding new features, it is very easy to forget to alter comments if they are present on most lines. You must be very alert if this has never happened to you.

If you alter a line of code, is it so hard to check the lines above and below it for the comment? What kind of programmer works on individual lines in isolation? A function is an integral unit of code, and when altering a function you should be familiar with all of it. (If that''s impractical, then your function is too large.) Really, maintaining comments is not difficult at all. I''ve been working on one project that contains over 150 C++ files for 4 years, and I never once recall coming across a comment that was out of date - even those I wrote in 1998!

If it''s hard to ensure a comment written in plain English is correct when you change other lines of code, then you have no chance of ensuring your C++ code is correct when you change those lines. (''You'' used in the general sense.)

[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
quote:
Original post by Kylotan
See, I consider these to be bad comments. If you add stuff to or remove stuff from the update routine, you now have to change the comment. I comment functions with a conceptual overview of what the function does - usually a one sentence description such as "//checks for status changes such as death, and processes movement based on keypresses". That tells you most of the same information and is less likely to become inaccurate.

Also, having big comment blocks at the top of a function is often a bad idea. When you alter line 50 of a function, you would then have to scroll up a page or two in order to find the comment that you might have to check. The documentation for a function should rarely be explaining everything that the function does - rather, it should explain the circumstances under which you would call it, and what the consequences are. I rarely find this takes more than a sentence. And with a well-designed system, this rarely changes.



It was probably a bad example but I think you should comment what your functions do this well. The person using your code may not have access to the code implementation of your function and they shouldn''t have to go through your code to understand it anyway. Also, what functions do shouldn''t change much (the way they do it can though), as that will break code using your functions. I''m thinking in more broader terms than a code writer using only his own code.

Could you paste a code example of your code so I can get an idea of the kind of commenting you do?

quote:

Your example of excessive commenting is indeed excessive.



Would you add comments in the function body though? In this simple example, I would consider any comments in the function body excessive because it is so readable.

quote:

Yeah, but now you''re saying "bad comments are a hassle to keep up to date". I say that, "good comments require next to no work to keep up to date". They usually impart information about the system at a high enough level that they are constant.



Yes, I agree with you there. I think a good summary is that low level comments are bad.

quote:

If you alter a line of code, is it so hard to check the lines above and below it for the comment? What kind of programmer works on individual lines in isolation? A function is an integral unit of code, and when altering a function you should be familiar with all of it. (If that''s impractical, then your function is too large.) Really, maintaining comments is not difficult at all. I''ve been working on one project that contains over 150 C++ files for 4 years, and I never once recall coming across a comment that was out of date - even those I wrote in 1998!



It is time consuming to have to check comments and code. You may not find it hard, but someone else using your code might.

quote:

If it''s hard to ensure a comment written in plain English is correct when you change other lines of code, then you have no chance of ensuring your C++ code is correct when you change those lines. (''You'' used in the general sense.)



I can write very good and correct C++ code and I have had out of date comments. I still don''t know the level at which you comment, but when you comment too much it is very time consuming to have to first check the C++ code is correct and then to check the comments are correct (which is difficult because the compiler ignores them).

Share this post


Link to post
Share on other sites
I''d like to see an example of your commenting style as well, Kylotan. You bring up some very good points. My commenting style has changed dramatically (for the better) since you''ve been posting on this thread. Seeing a good example of how you comment would be a big help.

Share this post


Link to post
Share on other sites
quote:
Original post by Anonymous Poster
It was probably a bad example but I think you should comment what your functions do this well. The person using your code may not have access to the code implementation of your function and they shouldn''t have to go through your code to understand it anyway.

Ok... conceptually, a function is a level of abstraction. This applies for almost every type of programming language. In order to use the function, you shouldn''t need to know the code implementation. If you do, then your design is flawed. All the caller of a function should ever need to know, is what to put into the function, and what they get out of the function (whether that is a return value, or a change of state). The only time they really need to know how it does it, is when they are going to alter that function itself. And in that case, they will need to be looking at the whole function code anyway. And if they don''t have access to the implementation, then why would they need info on how it''s implemented? They can''t change it, so such documentation is largely useless.

quote:
Would you add comments in the function body though? In this simple example, I would consider any comments in the function body excessive because it is so readable.

In that last example, the code may well be readable. The comment that gets out of date in that example seems bizarre to me, as you have just added 5 lines directly above it to the same if-chain. If you alter one part of a conditional statement, you absolutely have to check the rest of it for correctness. This includes any comments.

quote:
It is time consuming to have to check comments and code. You may not find it hard, but someone else using your code might.

Checking code is part of the process. Comments are just part of the code, as I see it. If you change code in one place, you check the code around it to ensure you''ve not invalidated it. Having to check a couple of extra comments each time is not going to cost you that much, and it will also help you hone your program''s correctness. Sometimes, after looking at an comment you''ve just rendered ''out-of-date'', you could realise that you shouldn''t have changed the code at all.

quote:
I still don''t know the level at which you comment, but when you comment too much it is very time consuming to have to first check the C++ code is correct and then to check the comments are correct (which is difficult because the compiler ignores them).

You are mixing up syntactic correctness, which the compiler checks, and logical correctness, which the compiler does not check, and cannot check in 99% of circumstances. The compiler can''t check your code to see if you really meant !IsDead() when you typed IsDead(). And at a glance, you might not notice this mistake either. But imagine this code:

// Send message if the character is dead
if (!ch.IsDead())
{
cout << "I died!";
}

Your first reaction might be that the comment is extraneous and that you can see what the code does just by looking at it. Everything is self-evident, after all. But, the code hides a bug in just a single character - that stray ! means that it does the exact opposite of what you expect. That comment contradicts the code, and so it alerts you to a possible bug - either the comment is wrong, or the code is wrong. So you fix one of them and your program has benefitted. By being more verbose, and being redundant, you have gained an extra level of safety-checking in your program. Comments state intention, code states action. Any discrepancies between the two are handy alarm bells telling you to double check your code.

Ok, here''s an example of some of my recent code for you to pick apart:
  
// Run the application - the main entry point

int Application::Run()
{
// Quick test


// Load a background image

ImageID titleScreen = theVideo->LoadImage("graphics/hill.bmp");

// Error check

if (titleScreen == INVALID_IMAGE_ID)
{
clog << "Error loading title screen." << endl;
return EXIT_FAILURE;
}

// Draw it to the screen

theVideo->DrawImage(titleScreen, 0, 0);
theVideo->Update();

// Take a screenshot

theVideo->SaveScreenShot("menuscreenshot.bmp");

// Wait 1 second

clock_t now = clock();
while (clock() < now + 1000)
;

// Run the game!

GameState theGame(*theVideo, *input);
theGame.Run();

// Wait 1 second

clock_t now = clock();
while (clock() < now + 1000)
;

// All done.


return EXIT_SUCCESS;
}

Now sure, some of those comments are ''redundant'' in that they don''t impart extra information, but they back up the intent of the code. They are one level above the implementation level, in that you can just read the comments to see what the function does. You can change the way the code works and the comments could largely remain unaltered. Maybe you have to remove a comment, or add one, but not change one. Yet they are close enough to the implementation to be able to check the code against them for correctness. Perhaps the only bad comment (in my opinion) is the first one, because it says very little - but then, little needed to be said about this function. You call it to run the game, and that''s that. It may be a redundant comment, but it''s not incorrect, and it''s not going to need to be changed either.

This probably isn''t the best example, but it''s not the worst either. I hope that my points about abstraction and checking correctness stand up regardless of whether my example passes scrutiny or not.

[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
quote:
Original post by Kylotan
Ok... conceptually, a function is a level of abstraction. This applies for almost every type of programming language. In order to use the function, you shouldn''t need to know the code implementation. If you do, then your design is flawed. All the caller of a function should ever need to know, is what to put into the function, and what they get out of the function (whether that is a return value, or a change of state). The only time they really need to know how it does it, is when they are going to alter that function itself. And in that case, they will need to be looking at the whole function code anyway. And if they don''t have access to the implementation, then why would they need info on how it''s implemented? They can''t change it, so such documentation is largely useless.



I understand your point, but the player example needs that information somewhere. It may not be obvious that the player dies when his health is less than zero (maybe you thought it would be <= 0) so it must be documented somewhere. Also the fact the player may change state here is important. The movement stuff would probably be commented somewhere else (such as in constructor parameters and what they are for), but it is still part of what the player is and can do and is not an implementation detail. It was just the example was so simple the function documentation was almost implementation documentation.

quote:

In that last example, the code may well be readable. The comment that gets out of date in that example seems bizarre to me, as you have just added 5 lines directly above it to the same if-chain. If you alter one part of a conditional statement, you absolutely have to check the rest of it for correctness. This includes any comments.



It probably was a silly example, but this is just an example of how they can go out of date, which you stated has "never" happened to you. I am an experienced programmer and, I admit, this will happen to me if I comment too much. It tends to happen when you are just making lots of quick changes (like integrating a trival feature which propagates about your code) and when part of other comments refer to others you have just changed.

quote:

You are mixing up syntactic correctness, which the compiler checks, and logical correctness, which the compiler does not check, and cannot check in 99% of circumstances. The compiler can''t check your code to see if you really meant !IsDead() when you typed IsDead(). And at a glance, you might not notice this mistake either. But imagine this code:

// Send message if the character is dead
if (!ch.IsDead())
{
cout << "I died!";
}

Your first reaction might be that the comment is extraneous and that you can see what the code does just by looking at it. Everything is self-evident, after all. But, the code hides a bug in just a single character - that stray ! means that it does the exact opposite of what you expect. That comment contradicts the code, and so it alerts you to a possible bug - either the comment is wrong, or the code is wrong. So you fix one of them and your program has benefitted. By being more verbose, and being redundant, you have gained an extra level of safety-checking in your program. Comments state intention, code states action. Any discrepancies between the two are handy alarm bells telling you to double check your code.



I understand your point, but the only way to add this extra safety is to comment at a lower level. This discussion needs more concrete examples, but I don''t think those kind of comments help too much. The only reason you''d be looking for bugs like that in the first place is if you were alerted to it by testing and higher level comments would still lead you to these kind of things anyway. Unit tests and play testing are much better at catching these type of things.

Although, I do admit that when you comment like this, it makes you think about what you''ve type in a bit more. Design does this also though.

quote:

Ok, here''s an example of some of my recent code for you to pick apart:

    
// 1 Run the application - the main entry point

int Application::Run()
{
// 2 Quick test


// 3 Load a background image

ImageID titleScreen = theVideo->LoadImage("graphics/hill.bmp");

// 4 Error check

if (titleScreen == INVALID_IMAGE_ID)
{
clog << "Error loading title screen." << endl;
return EXIT_FAILURE;
}

// 5 Draw it to the screen

theVideo->DrawImage(titleScreen, 0, 0);
theVideo->Update();

// 6 Take a screenshot

theVideo->SaveScreenShot("menuscreenshot.bmp");

// 7 Wait 1 second

clock_t now = clock();
while (clock() < now + 1000)
;

// 8 Run the game!

GameState theGame(*theVideo, *input);
theGame.Run();

// 9 Wait 1 second

clock_t now = clock();
while (clock() < now + 1000)
;

// 10 All done.


return EXIT_SUCCESS;
}



I''ve numbered the comments to make it easier to see what I''m refering too. I realise this is an example so don''t take this too seriously like I''m attacking your coding style (which is nice by the way):

1. I''d add what the return type was as well here.

3., 5., 6., 8. I think they add no extra information.

4. I would use exceptions because this seperates error code and normal code and makes code much cleaner. I''d then add what was thrown to 1. Everytime you load an image you''ll have to write this kind of conditional statement and handle the error. Even if you rememer to do so, you are writing more code that needs to be check and you might also makes mistakes.

7., 9. I''d put this in a function called waitOneSecond or use a more generic timer class with readable names. Although, it is a standard function so anyone worth their salt would know what it does or they could just look it up. Changing the 1000 to ONE_SECOND or something similar would also be helpful as well.

10. Should be able to know what this means from 1.

quote:

Now sure, some of those comments are ''redundant'' in that they don''t impart extra information, but they back up the intent of the code. They are one level above the implementation level, in that you can just read the comments to see what the function does. You can change the way the code works and the comments could largely remain unaltered. Maybe you have to remove a comment, or add one, but not change one. Yet they are close enough to the implementation to be able to check the code against them for correctness. Perhaps the only bad comment (in my opinion) is the first one, because it says very little - but then, little needed to be said about this function. You call it to run the game, and that''s that. It may be a redundant comment, but it''s not incorrect, and it''s not going to need to be changed either.



Ok, here''s how I would do it then so you can pick apart what you think is bad:

  
// Executes game application and also takes a screenshot of the title screen.

//

// Throws - FileNotFoundException if title screen image not found.

void GameApplication::Run()
{
theVideo->DrawImage(theVideo->LoadImage(FILENAME_IMAGE_TITLE_SCREEN));
theVideo->Update();
theVideo->SaveScreenShot(FILENAME_IMAGE_SCREENSHOT);

waitOneSecond();
GameState(*theVideo, *input).Run();
waitOneSecond();
}


I got rid of titleScreen because it would also free up the memory it uses once you display it. I got rid of the "0, 0" parameters because I think having an overloaded DrawImage which displays the whole image to the screen documents what you are doing.

quote:

This probably isn''t the best example, but it''s not the worst either. I hope that my points about abstraction and checking correctness stand up regardless of whether my example passes scrutiny or not.



Don''t worry, I''m sure my next post will contain the line "isn''t the best example" too

Share this post


Link to post
Share on other sites
quote:
Original post by Anonymous Poster
I understand your point, but the only way to add this extra safety is to comment at a lower level. This discussion needs more concrete examples, but I don''t think those kind of comments help too much. The only reason you''d be looking for bugs like that in the first place is if you were alerted to it by testing and higher level comments would still lead you to these kind of things anyway. Unit tests and play testing are much better at catching these type of things.

I disagree. The earlier in the process you catch a bug, the more time you save by doing so. Being able to read the code and quickly match up fact with intent is a tool that lets you check correctness very quickly. In the book ''Code Complete'', Steve McConnell cites evidence that shows that reading through your code is more effective at finding defects than tests are.

Your comments on my comments -
1. I''d add what the return type was as well here.
Fair enough. In this case, this function is effectively the same as ''main()'' and main has standard return types which I adhere to. Whether you comment standard behaviour is going to vary depending on your situation.

3., 5., 6., 8. I think they add no extra information.
3 tells you what the image is going to be used for without you having to read further into the code. This sets up expectations of the rest of the code making it easier to see if something was overlooked.

5 makes it clear that theVideo object refers to the screen, as it may not be obvious that calling ''theVideo->DrawImage'' writes to the screen. Many blit routines require you to specify a destination surface.

6 - you''re probably right. But removing that line if and when I remove the screenshot-taking line costs me virtually nothing, so the comment is ''free''.

8 - again, you''re probably right, but if this function became longer and more detailed, it would be handy to have the ''important'' line marked with a comment so that you can easily spot all the code that comes before the game is run and all the code that comes after the game is run. This is a ''forward-looking'' comment, if you like.

4. I would use exceptions because this seperates error code and normal code and makes code much cleaner.

I wasn''t aware we were discussing exceptions? What constitutes an ''exceptional condition'' is very subjective so I won''t discuss it here. I use exceptions elsewhere in that program.

quote:
Everytime you load an image you''ll have to write this kind of conditional statement and handle the error. Even if you rememer to do so, you are writing more code that needs to be check and you might also makes mistakes.

In principle, this is true, but in this case, all the uses of ImageID were enumerated up front so I knew that only 3 routines needed to handle this problem. In particular, the only thing you can do with an ImageID is pass it to DrawImage which checks its validity there and then.

7., 9. I''d put this in a function called waitOneSecond or use a more generic timer class with readable names.
This is prototype code and will be removed from the final project. Thus, effort was taken to comment it as an alternative to writing special wrapper code or classes for it.

10. Should be able to know what this means from 1.
Shouldn''t have to refer to 1 to know what 10 means I believe in keeping information local to where it is relevant.

Regarding your example, I think that the style is fine, given that it''s a short function. However, I would not nest so many functions in the way that you do. That is, I program in a procedural style whereas you use a functional style. Functional style is more compact, but makes it harder to follow, and certainly harder to see what''s happening when you step over it in a debugger. It also means you have to refactor the code if and when you need to add intermediate commands. What if you need to call some other method for that GameState object before you call Run()? Perhaps some extra initialisation, or just some sort of diagnostic function... You will have to split up that one line into 2, just so that you can add the 3rd in between them. The conciseness saves you some typing now, but costs you in maintenance time. Again, ''Code Complete'' has a section suggesting to use only 1 statement per line, citing this and other problems. For example, some compilers even find it easier to optimise code that spans several lines compared to code all gathered in one statement.

Additionally, I would not throw the exception from the function, I would catch and handle it within that function, but that''s just how I would prefer to handle it.

quote:
I got rid of titleScreen because it would also free up the memory it uses once you display it.

Actually, it doesn''t. The image data stays loaded until you call FreeImage or destroy the Video object, and the ImageID type is allocated on the stack and will thus persist until the function concludes anyway. (It is also of trivial size (4 bytes) but a maintenance programmer may not know that.) Minor point.

quote:
I got rid of the "0, 0" parameters because I think having an overloaded DrawImage which displays the whole image to the screen documents what you are doing.

I only agree with default arguments or overloaded functions where the overloaded version is going to be used so commonly that the convenience outweighs the difficulty of needing to remember the special cases. In this case, I would disagree, as no other part of the program would ever need to call this function with the parameters 0,0 (except the rare case of a tile being drawn at that position). So I would say that this obfuscates the code a little by making the interface more complex than it needs to be. Another minor point though.


[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
quote:
Original post by Kylotan
I disagree. The earlier in the process you catch a bug, the more time you save by doing so. Being able to read the code and quickly match up fact with intent is a tool that lets you check correctness very quickly. In the book ''Code Complete'', Steve McConnell cites evidence that shows that reading through your code is more effective at finding defects than tests are.



I agree they help catch bugs, but I still think you should only do high-level comments. If you use appropriate variable names, function names and keep your functions short, it should be very easy to follow what is (or isn''t in the case of a bug) going on.

quote:

Fair enough. In this case, this function is effectively the same as ''main()'' and main has standard return types which I adhere to. Whether you comment standard behaviour is going to vary depending on your situation.



Well, you''d have to comment that it is standard behavour somewhere. I think the use of exceptions made it cleaner though by getting rid of this return type. I would call the game failing exceptional.

quote:

3 tells you what the image is going to be used for without you having to read further into the code. This sets up expectations of the rest of the code making it easier to see if something was overlooked.



I think the name "titleScreenImage" should be enough for to be able to work out that it is an image that will be used as a title screen. The comment said it was an image (known by variable type), it is loaded (known by Load word in method) and it is some kind of background to something.

quote:

5 makes it clear that theVideo object refers to the screen, as it may not be obvious that calling ''theVideo->DrawImage'' writes to the screen. Many blit routines require you to specify a destination surface.



Maybe you should change "theVideo" to "screen" then to make it more obvious. When I had these type of classes I recall using statements like video.getDoubleBuffer().draw(titleScreen).

quote:

8 - again, you''re probably right, but if this function became longer and more detailed, it would be handy to have the ''important'' line marked with a comment so that you can easily spot all the code that comes before the game is run and all the code that comes after the game is run. This is a ''forward-looking'' comment, if you like.



If it became too long for this to be obvious, I would probably split it up a bit. If you just quickly read through the code (like you''d have to read through the comments) it would still be easy to pick up on I think.

quote:

I wasn''t aware we were discussing exceptions? What constitutes an ''exceptional condition'' is very subjective so I won''t discuss it here. I use exceptions elsewhere in that program.



Fair enough I just consider not finding a file exceptional because I can''t think of any situations where I would except a file I wanted to open not being there. Makes the code a lot cleaner in my opinion.

quote:

In principle, this is true, but in this case, all the uses of ImageID were enumerated up front so I knew that only 3 routines needed to handle this problem. In particular, the only thing you can do with an ImageID is pass it to DrawImage which checks its validity there and then.



It''s just that when I write these kind of classes I try to think of longer term use. If you started using it a lot, you might become lazy and just assume the file was loaded and your code would be less robust.

quote:

Shouldn''t have to refer to 1 to know what 10 means I believe in keeping information local to where it is relevant.



It isn''t that much hassle to read the function documentation (it should only be one page up maximum). If you had many places with "return EXIT_SUCCESS" you would then have to follow your style and say why you are returning this value and you are then just duplicating your comments, thus they are more likely to be inconsistant. Something alongs the lines of "Returns - EXIT_SUCCESS when game finish under normal circumstances" would be ok.

quote:

Regarding your example, I think that the style is fine, given that it''s a short function. However, I would not nest so many functions in the way that you do. That is, I program in a procedural style whereas you use a functional style. Functional style is more compact, but makes it harder to follow, and certainly harder to see what''s happening when you step over it in a debugger. It also means you have to refactor the code if and when you need to add intermediate commands. What if you need to call some other method for that GameState object before you call Run()? Perhaps some extra initialisation, or just some sort of diagnostic function... You will have to split up that one line into 2, just so that you can add the 3rd in between them. The conciseness saves you some typing now, but costs you in maintenance time. Again, ''Code Complete'' has a section suggesting to use only 1 statement per line, citing this and other problems. For example, some compilers even find it easier to optimise code that spans several lines compared to code all gathered in one statement.



By the way, I''m not a fan of programming in a compact style (i.e. cram as much onto one line as possible). Also, the post deleted some of my whitespace About two statements a line is about as much as I''ll go.

It is said to be more unreadable having more than one statment a line but I find the "DrawImage" line quite readable. "Draw the loaded title screen image to the screen."

I get your point with the refactoring but it isn''t much effort. You said earlier you like keeping information local and the "GameState" and "LoadImage" line clearly show that both of those object are only needed for that one point.

It may be faster to type but I cannot think even close to as fast as I type. More than one statement a line is not done for this reason. On occasion, I just find it makes it more readable than having to introduce variable names that are only used once. Having to refactor this doesn''t take much time compared to the thought involved.

quote:

Additionally, I would not throw the exception from the function, I would catch and handle it within that function, but that''s just how I would prefer to handle it.



I''d probably have a try/catch around where the GameApplication::Run method is called so I could effciently (in coding terms) catch all fatal errors (not just file that aren''t there) from the game and report them to the user in one place. Otherwise you have to have return types for all your functions, you''ve got to check them all manually etc.

quote:

Actually, it doesn''t. The image data stays loaded until you call FreeImage or destroy the Video object, and the ImageID type is allocated on the stack and will thus persist until the function concludes anyway. (It is also of trivial size (4 bytes) but a maintenance programmer may not know that.) Minor point.



I would try to design it to happen in a destructor somewhere. Less hassle, less code and less behaviour to comment

quote:

I only agree with default arguments or overloaded functions where the overloaded version is going to be used so commonly that the convenience outweighs the difficulty of needing to remember the special cases. In this case, I would disagree, as no other part of the program would ever need to call this function with the parameters 0,0 (except the rare case of a tile being drawn at that position). So I would say that this obfuscates the code a little by making the interface more complex than it needs to be. Another minor point though.



Minor point, yes. I just looked at the two zeros and thought that it meant the top-left of the screen and then thought that wasn''t important here because you just wanted it drawn to the whole screen. You might only use it a few times, but if you reuse your code, you''ll probably use it more.

By the way, one thing that really bugs me when I''m trying to comment things is that comments don''t have much in the way of scope. If I comment a line, does it comment the next line, the next block etc. You could say "any comment is effective to the next lines not seperated by a blank line" but that means you have to bunch up things together, which sometimes looks nasty and hard to read. I''m not saying you shouldn''t comment because of this, it''s just something that annoys me when I do

Also (prompted by your sig.), have you ever looked at the Boost library source code? These libraries are being considered for the next standard so these guys are obviously very good. Trying to read the code though is an absolute nightmare! I think they should comment more

Share this post


Link to post
Share on other sites
I''ll not respond to most of these points, as we''re not going to change each other''s mind, and hopefully anyone else still reading this point has got a good idea of our respective positions, and can make up their own minds on what to do.

quote:
Original post by Anonymous Poster
Maybe you should change "theVideo" to "screen" then to make it more obvious.

That''s something I''ve considered, but the Video class does more than just manage the screen so I''ve kept it separate for now.

quote:
If you had many places with "return EXIT_SUCCESS" you would then have to follow your style and say why you are returning this value and you are then just duplicating your comments, thus they are more likely to be inconsistant. Something alongs the lines of "Returns - EXIT_SUCCESS when game finish under normal circumstances" would be ok.

There shouldn''t be many places really - only the Application::Run function as far as I know. Maybe a different design could require the various values to decided deeper in the code, and in that case some sort of exception to signal EXIT_FAILURE would be fine.

quote:
I''d probably have a try/catch around where the GameApplication::Run method is called so I could effciently (in coding terms) catch all fatal errors (not just file that aren''t there) from the game and report them to the user in one place. Otherwise you have to have return types for all your functions, you''ve got to check them all manually etc.

I actually have exactly that - int main() has a try/catch block in it, and that is where Application::Run is called. So if I wanted missing files to start being exceptional conditions, I would be able to do this quite easily. However, at the moment I want the program to run even if it can''t find a file.

I also tend to reduce my reliance on exceptions because writing truly exception-safe code is a nightmare. (See how many Guru Of The Week articles have been devoted to this issue as evidence.)

quote:
I would try to design it to happen in a destructor somewhere. Less hassle, less code and less behaviour to comment

Images are freed in the Video class''s destructor. Or you can manually free them with FreeImage if you like.

quote:
Also (prompted by your sig.), have you ever looked at the Boost library source code? These libraries are being considered for the next standard so these guys are obviously very good. Trying to read the code though is an absolute nightmare! I think they should comment more

You''re probably right A lot of good programmers write horrible code. It''s likely that they only keep their jobs because their employer fears that it would take months before their replacement would understand the source for whatever they were working on. Oh well.



[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost ]

Share this post


Link to post
Share on other sites

  • Advertisement