Skip to main content
GameDev.net gamedev.net
🔒 Locked

Clean Coding

Started by UberJim Jan 30, 2005 at 1:58 PM 23 replies 4.5k views
Original Post
UberJim
UberJim
I'm not completely new to programming, just new to clean coding. I started programming with QBasic ages ago, and now I'm trying to improve my C++ skills. I'm no expert, but I have gone as far as making an extremely dirty Tic-Tac-Toe game. Now I am pretty much starting my C++ from scratch so that I do everything right. Here is some sample code to an extremely simple program that I made, and I would like some input on how close I am to clean code, and how I should improve it.
Quote:
//Average Score //A simple program that prompts the user for 3 scores, then displays the average of them //Created by Jim on 01/30/05 #include using std::cout; using std::cin; using std::endl; int score1 = 0; int score2 = 0; int score3 = 0; int main() { cout << "Enter the first score: "; cin >> score1; cout << "\nEnter the second score: "; cin >> score2; cout << "\nEnter the third score: "; cin >> score3; cout << "\nThe average of the 3 scores is " << (score1 + score2 + score3) / 3 << endl; system("PAUSE"); return 0; }
I am working with a book titled "Beginning C++ Game Programming" authored by Michael Dawson. It seems pretty good so far, a lot easier to follow than the textbook I used in high school. Edit: Hmm, after looking at my post I see that "extremely dirty" could easily be taken in a different light, I meant the code was just sloppy.
Kitt3n
Kitt3n
Looks pretty nice - I'd only limit the scope of the three
integers to the main function (that is, put the three int's
inside the main function, no need to have them global!)

Usually it's a good idea to limit the scope of variables
as much as possible :)
visit my website at www.kalmiya.com
UberJim
UberJim
Quote:
Original post by Kitt3n
Looks pretty nice - I'd only limit the scope of the three
integers to the main function (that is, put the three int's
inside the main function, no need to have them global!)

Usually it's a good idea to limit the scope of variables
as much as possible :)


Thanks for the tip. That is exactly the kind of feedback I wanted.
dave
dave
Ok, so i know this is pedantic but this is how i would change it if perfection was important.

//Average Score//A simple program that prompts the user for 3 scores, then displays the average of them//Created by Jim on 01/30/05#include <iostream>using namespace std;namespace // <-- This file has its own namespace, use this if file scope global is required{	const unsigned int	DEFAULT_ZERO			=	0;	const unsigned int	SCORE_INIT_VALUE		=	DEFAULT_ZERO;	const unsigned int	TOTAL_SCORES			=	3;	const unsigned int	SCORE_LOOP_START		=	DEFAULT_ZERO;	const unsigned int	SCORE_LOOP_END			=	TOTAL_SCORES;	const char			SCORE_ENTER_PHRASE[]	=	"Enter score ";	const char			OUTPUT_MESSAGE[]		=	"\nThe average of the scores is ";}	int main(int argc, char* argv[]) // caters for possible command line args{	int score[TOTAL_SCORES];	unsigned int scoreIterator;	int totalScore = 0;	for (	scoreIterator = SCORE_LOOP_START;			scoreIterator < SCORE_LOOP_END;			scoreIterator++)	{		cout << SCORE_ENTER_PHRASE << scoreIterator << endl;		cin	>> score[scoreIterator];		totalScore += score[scoreIterator];	}		cout << OUTPUT_MESSAGE << (totalScore / TOTAL_SCORES)       << endl;	system("PAUSE");	return 0;}


I know some of this may seem like overkill but that may well be because it is a good habit to get into.

Also im aware mine is more functional than urs is, just got carried away =)

ace

[Edited by - ace_lovegrove on January 30, 2005 2:23:33 PM]
UberJim
UberJim
Quote:
Original post by ace_lovegrove
Another pedantic thing might be to make a good habbit of making the main declaration as:

int main(int argc, char* argv[])
{
return 0;
}


That doesn't mean anything to me without a reason.
Drakkcon
Drakkcon
No actual coding tips, but use [ code ] [ /code ] tags (without the spaces) to format correctly, so every space is registered. It also puts it in nice 
monospace
font.

without tag:

while (1)
{
cout << "Teh ugly";
}

with tag:
while (1){  cout << "Teh h0t";}
Drakkcon
Drakkcon
Oh, and what Ace said is true. Many programs in dos require arguments: like
hello.exe -b -g -h . You use his form to keep track of these:
argc is the number of arguments, and
argv[] is a character array with these arguments, so you can use them.
dave
dave
Quote:
Original post by Drakkcon
Oh, and what Ace said is true. Many programs in dos require arguments: like
hello.exe -b -g -h . You use his form to keep track of these:
argc is the number of arguments, and
argv[] is a character array with these arguments, so you can use them.


char* argv[] is actually a pointer to an array of pointers to the command line arguments.

ace
Dakar
Dakar
Alos, I would consolidate the three variable lines into one: 
int score1, score2, score3;


Then, you could do something like 
score1 = score2 = score3 = 0;
.
andrewo
andrewo
Quote:
Original post by Dakar
Alos, I would consolidate the three variable lines into one: 
int score1, score2, score3;


Then, you could do something like 
score1 = score2 = score3 = 0;
.


I disagree with this advice. Placing each variable declaration on its own line greatly increases readability.

When one attempts to fit multiple variable declarations on a line, it detracts from readability and clutters code - effectively rendering it less manageable.

Consider this:
int x, y;int width, height;Mammal dog, cat, monkey, lion, tiger;Reptile lizard, alligator;Amphibian frog, toad;


I consider that to be a whole lot less readable and a lot less manageable than this:
int x;int y;int width;int height;Mammal dog;Mammal cat;Mammal monkey;Mammal lion;Mammal tiger;Reptile lizard;Reptile alligator;Amphibian frog;Amphibian toad;


This style is a lot more manageable and allows the coder to more easily identify a variable and its type. You can easily tell that the toad is an Amphibian, not a Reptile. This distinction isn't as obvious in the first example.

Sorry, I know this is a small nitpick but this may save you a headache or two later on. Likewise, it's always easier to start off with good coding habits than to later switch to them.
Brandon N
Brandon N
Quote:
Original post by andrewo
Quote:
Original post by Dakar
Alos, I would consolidate the three variable lines into one: 
int score1, score2, score3;


Then, you could do something like 
score1 = score2 = score3 = 0;
.


I disagree with this advice. Placing each variable declaration on its own line greatly increases readability.

When one attempts to fit multiple variable declarations on a line, it detracts from readability and clutters code - effectively rendering it less manageable.

Consider this:
int x, y;int width, height;Mammal dog, cat, monkey, lion, tiger;Reptile lizard, alligator;Amphibian frog, toad;


I consider that to be a whole lot less readable and a lot less manageable than this:
int x;int y;int width;int height;Mammal dog;Mammal cat;Mammal monkey;Mammal lion;Mammal tiger;Reptile lizard;Reptile alligator;Amphibian frog;Amphibian toad;


This style is a lot more manageable and allows the coder to more easily identify a variable and its type. You can easily tell that the toad is an Amphibian, not a Reptile. This distinction isn't as obvious in the first example.

Sorry, I know this is a small nitpick but this may save you a headache or two later on. Likewise, it's always easier to start off with good coding habits than to later switch to them.

I find having to scroll a page to read code makes it less readable. Your second version would significantly increase the likelihood of having to scroll. It unnecessarily makes the code longer.

Furthermore, anyone not using intellisense these days is just wasting their own time. You should never be forced to find the declaration of a variable to determine its type.
andrewo
andrewo
Quote:
Original post by Brandon N
I find having to scroll a page to read code makes it less readable. Your second version would significantly increase the likelihood of having to scroll. It unnecessarily makes the code longer.

Furthermore, anyone not using intellisense these days is just wasting their own time. You should never be forced to find the declaration of a variable to determine its type.


This seems like a rather inane thing to disagree over, but I strongly suggest using the one declaration per line rule. True, this style requires significantly more screen space, but it's certainly warranted, just as breaking a long expression down into multiple lines is necessary to preserve readability.

Also, this slighlty more verbose style allows one to more easily identify the variable's type. Also note that it is just as ridiculous to rely exclusively on intellisense to identify a variable as it is for one to be forced to find the declaration of the variable to determine its type. Both means reinforce to the reader the variable's type. Remember, however, that to a person that reads the code, the one declaration per line rule is far more readable.

The cost of scrolling in this example is outweighed by the increased readability of the declarations. A person reviewing a routine can far more easily identify the variables when they are declared on a line by themselves.

Certainly, this is a rather subjective style consideration, though.
UberJim
UberJim
Thanks for all of the tips. Do you think you should have "using namespace std" when you are only using a couple things out of it? Wouldn't that load things that aren't needed into memory?
load_bitmap_file
load_bitmap_file
Quote:
Original post by UberJim
Thanks for all of the tips. Do you think you should have "using namespace std" when you are only using a couple things out of it? Wouldn't that load things that aren't needed into memory?


Yeah, you shouldn't say "using namespace std" considering the std namespace has a LOT of things in it, and you'll only be using a small part of it. (Other stuff can happen as well, like name clashes when doing using namespace whatever)

Quote:
Original post by ace_lovegrove
Quote:
Original post by Drakkcon
Oh, and what Ace said is true. Many programs in dos require arguments: like
hello.exe -b -g -h . You use his form to keep track of these:
argc is the number of arguments, and
argv[] is a character array with these arguments, so you can use them.


char* argv[] is actually a pointer to an array of pointers to the command line arguments.

ace


Actually, Drakkcon was right. argv is an array of char pointers(C-style strings), not the other way around.
Zahlman
Zahlman
Quote:
Original post by ace_lovegrove
Ok, so i know this is pedantic but this is how i would change it if perfection was important.

(Edit: Apparently the reason the board removes source snippets in quotes is that they mess with the quote tags. Grr.)

I know some of this may seem like overkill but that may well be because it is a good habit to get into.

Also im aware mine is more functional than urs is, just got carried away =)

ace


I'm crying here, and not in a good way.

You made a namespace for "file scope globals", introducing the possibility of other people putting extra things into your namespace, when all you really need to do to keep them "file scope" is not mention them in the header.

You made a completely useless constant to represent the zero value. It is not sane to suppose that there will ever be a need to change "DEFAULT_ZERO" in the future, and it adds nothing to readability. Plus, you prove the unintuitiveness of using it consistently, when you write "int totalScore = 0;", forgetting all about your SCORE_INIT_ZERO.

You separate out const char[]'s to represent your text. Okay, I understand the usefulness of moving those into a common location for internationalization purposes - but - optimization shouldn't be a concern at this point; use a real textual type to represent text data - i.e. std::string. Since we're talking internationalization here, it pays to remember that Unicode might become an issue. Outputting a const char[] isn't going to magically convert from UTF-8, you know.

You lost the functionality of labelling the score values "naturally" with english names. Heck, you expect the user not to balk at "Enter score 0".

You accumulate the user information into a useless array (writing to each location once and reading back once). I don't complain on grounds of optimization, but on the grounds that doing that work obfuscates matters - someone reading the code wonders why the array is there.

You missed the opportunity to encapsulate the process of prompting for/getting input, and to ensure that a number is entered.

You missed the opportunity to account for round-off error (integer division discards the remainder).

You kept in the system("PAUSE") call, which doesn't properly belong in console apps, but in a batch file that runs the app (Fruny can sell this better than I).

#include <iostream>#include <limits>using namespace std;// Text used by the program.const string enter = "Enter the ";const string[] const ordinals = {"first", "second", "third"};const string score = " score:";const string error = "That is not a valid score; please enter a number."const string theAverageIs = "The average of the 3 scores is ";// The use of 'sizeof' here is an idiom which determines the length of an// array at compile-time. In the future, when we add more labels to 'ordinals',// we will therefore not have to count them; the compiler does it for us :)const int numScores = sizeof(ordinals) / sizeof(string);int getInput(string prompt) {  int result;  // At the beginning of each loop, write the prompt, then read input.  // The output will always succeed; then we check if the input failed as  // the condition for looping.  while ((cout << enter << prompt << score) && !(cin >> result)) {    // Each time through this loop, we were given a non-numeric input.    // So we must write an error message,    cout << endl << error << endl;    // Fix up cin so that we can read again,    cin.clear();    // and skip over the bad input so we don't read it again.    cin.ignore(numeric_limits<streamsize>::max(),'\n');    // Now the loop will execute again, showing the prompt and getting new input.  }  cout << endl; // put space before the next prompt (or answer)  // Otherwise, result contains what we need.  return result;}int main(int argc, char* argv[]) {  int sum = 0;  for (int i = 0; i < numScores; i++) {	    sum += getInput(ordinals);  }  // In my own code, I would not use these two temporary variables, but instead  // just put the expressions right in line in the cout output. This is because  // I am comfortable with that sort of thing; I appreciate that beginners  // generally find it easier to give names to every quantity in the program.  // That's perfectly good practice if your names are good :)  int average = sum / numScores;  // The modulus ('%') operator returns the remainder in an integer division.  int remainder = sum % numScores;  cout << theAverageIs << average << " " << remainder << "/" << numScores << endl;}


Good code is at once readable and short, doing everything that it needs to do in only the right places (and avoiding redundancy). My code above only looks long because I have put in a large number of explanatory comments for instructive purposes (I wouldn't personally comment anything except maybe the while loop in getInput()) and because of a couple extra temporary variables (and the separation of the text data out for easier access). Oh, and because of the busy-work you have to do to read numbers in robustly in C++. (See this page for reference on this kind of stuff.)
UberJim
UberJim
Quote:
Original post by Zahlman
using namespace std;


Could you do that same thing above without "using namespace std", and doing about 4 "using std::blah" lines? How much memory does this actually save? I figure if it is that easy to save memory, why wouldn't you do it? Especially if you ever plan to program for anything other than high-end computers.
jsacret
jsacret
instead of system("pause") use

cin.get();

it helps to make code more portable
iMalc
iMalc
Don't worry Zahlman, it made me cringe too.

I really don't mean to be mean Ace, but your answers are furthur from "perfection" than you think.

There is such a thing a planning way too far ahead. I've seen it all too many times where people code in 10x more flexability than needed in a module which was never going to be of much use to begin with. These people need to focus on the task at hand. By all means, stay away from bad design practices and don't unnecessarily limit your options, but don't go off on a tangent either. Chances are that the direction you need to expand in later, inevitably wont be one of the ten you allowed for anyway. Not to mention that all that extra code, probably means exta bugs.

I'd much prefer to maintain UberJim's original code (perhaps after taking Kitt3n's advice) any day.

Also, I don't want to start a flame war, but I do much prefer declarations of the same type on the same line (until the line reaches the page width). Especially if all the variables in the function are of the same type anyway!
jfclavette
jfclavette
Quote:
Original post by jsacret
instead of system("pause") use

cin.get();

it helps to make code more portable


No no no no. Use nothing of that. A console app should be run from a console. When it's over, it's over and it should return. Like it has been mentionned, If you really need a "Wait" functionnality, put a PAUSE in the btchfile used to run the app.

I teleported home one night; With Ron and Sid and Meg; Ron stole Meggie's heart away; And I got Sydney's leg. <> I'm blogging, emo style
Zahlman
Zahlman
Quote:
Original post by UberJim
Quote:
Original post by Zahlman
using namespace std;


Could you do that same thing above without "using namespace std", and doing about 4 "using std::blah" lines? How much memory does this actually save? I figure if it is that easy to save memory, why wouldn't you do it? Especially if you ever plan to program for anything other than high-end computers.


The only difference that either way makes to "memory" is the storage space used for your actual source code. using-declarations only matter to the compiler when it is trying to figure out whether e.g. 'vector' means std::vector or your own 3D vector class. None of that information is present in the compiled project.

It's something you type for convenience. If it causes a problem with compilation, you can always switch it to import a symbol at a time.

@iMalc: There's more to it than overdesign. :) See added functionality and modularity in my version (input checking, reporting the remainder). Generally speaking, good design comes IMHO from an innate sense that is developed by experience - the experience of trying to innovate good design, rather than blindly copying whatever boilerplate was standard practice on your last project. This is just another of the many ways in which control-v is not your friend. :)

WRT the variable declarations, I usually group declarations of similar type and purpose on a line, and otherwise separate:
int x, y, w, h; // bounding boxint r, g, b; // colour

Except that I always separate the variables onto separate lines if they are pointers, because my coding 'morals' require that those little stars go on the type and not on the variable names in declaration:
int* a, b; // oops! b is an int, not an int*// I would have to do this:int* a, *b;// But dammit, the variable's name is b, and its type is int*. Why should I put// that extra * in the middle of nowhere? No, I'll stick with:int* a;int* b;// and curse at C for handing this legacy on to C++...

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.