Thread: comment on my coding style and if i use functions well

Code:

#include <iostream.h>
#include <stdlib.h>
#include <time.h>
//-------------------------------------------------------------------------------------------------
void CompRemove(int &totalstones)
/* determine the amount of stones the computer will remove */
{
	int compstones=30;
	
	while((compstones>3)||(compstones<1))
	{
		srand(time(NULL));
		compstones=((rand() % 3)+1);
	
		if ((totalstones-compstones)>=0)
		{
		totalstones=totalstones-compstones;	
		cout<<compstones<<endl;	
		}
		
		if ((totalstones-compstones)<=0)
		{
			CompRemove(totalstones);
		}	
	}
}
//-------------------------------------------------------------------------------------------------
int GenerateStones()
/* generate the amount of stones to start out with */
{
	int totalstones;

	srand(time(NULL));

	totalstones=((rand() % 15)+15);

	return (totalstones);
}
//-------------------------------------------------------------------------------------------------
void PlayerRemove(int &totalstones)
/* remove a stone between 1-3 and check to make sure value of total is not 0 */
{
	int playerstones;
	
	while(((playerstones>3)||(playerstones<1))||((totalstones-playerstones)<=0))
	{
		cin>>playerstones;

		if ((playerstones>3)||(playerstones<1))
		{
			cout<<"Value must be between 1 and 3"<<endl;
			cout<<"How many would you like? ";
		}

		if ((totalstones-playerstones)<=0)
		{
			cout<<"Value must be between 1 and "<<totalstones<<endl;
			cout<<"How many would you like? ";
		}
	}
	totalstones=totalstones-playerstones;
}
//-------------------------------------------------------------------------------------------------
void PlayGame()
/* start the game and keep track of all the variables during gameplay */
{
	int totalstones=GenerateStones();
			
	while(totalstones!=0)
	{
		cout<<"There are "<<totalstones<<" stones. How many would you like? ";
		PlayerRemove(totalstones);
		cout<<"There are "<<totalstones<<" stones. The computer takes ";
		CompRemove(totalstones);
	}
}
//-------------------------------------------------------------------------------------------------
int main()

{
	PlayGame();

	return (0);
}

whitespace is bad, do it like this:

Code:

#include <iostream.h>
#include <stdlib.h>
#include <time.h>
void CompRemove(int &totalstones) /* determine the amount of stones the computer will remove */ { int compstones=30; while((compstones>3)||(compstones<1)){srand(time(NULL));compstones=((rand() % 3)+1);if ((totalstones-compstones)>=0){totalstones=totalstones-compstones;cout<<compstones<<endl;	}if ((totalstones-compstones)<=0){CompRemove(totalstones);}}}int GenerateStones()/* generate the amount of stones to start out with */{int totalstones;srand(time(NULL));totalstones=((rand() % 15)+15);return (totalstones);}void PlayerRemove(int &totalstones)/* remove a stone between 1-3 and check to make sure value of total is not 0 */{int playerstones;while(((playerstones>3)||(playerstones<1))||((totalstones-playerstones)<=0)){cin>>playerstones;if ((playerstones>3)||(playerstones<1)){cout<<"Value must be between 1 and 3"<<endl;cout<<"How many would you like? ";}if ((totalstones-playerstones)<=0){cout<<"Value must be between 1 and "<<totalstones<<endl;cout<<"How many would you like? ";}}totalstones=totalstones-playerstones;}void PlayGame()/* start the game and keep track of all the variables during gameplay */{int totalstones=GenerateStones();while(totalstones!=0){cout<<"There are "<<totalstones<<" stones. How many would you like? ";PlayerRemove(totalstones);cout<<"There are "<<totalstones<<" stones. The computer takes ";CompRemove(totalstones);}}int main(){PlayGame();return (0);}

you are kidding right? right?

Yes, Brian is joking. You can always count on him for the wise-ass comment.

Originally posted by SilentStrike
Generally the comments about what a function does goes before the function. Also, you usually don't want to have any numbers ("magic numbers") appear in your program, other than 0 and 1. Everything use should be a named constant.

IE,

Code:

const int MAX_COMP_STONES=3;
const int MIN_COMP_STONES=1;

// code...
while (compstones > MAX_COMP_STONES || compstones < MIN_COMP_STONES) 
{ // etc

wow thx for the good advice sounds like a good practice i will make sure to implement it in my programs now thx!

looks good, easily readable and efficient (I think)
I still dont know whether <iostream> or <iostream.h> is better though

>I still dont know whether <iostream> or <iostream.h> is better though

I suppose better is a subjective argument, but the one the standards commitee thinks is better is <iostream>.

I need to work on this topic as well. My problem is I don't comment enough, and I hard code things too often.

I'd comment some variables. GenerateStones() seems like more work than necessary. You could randomize what you need in PlayGame(), save a function call and several lines of code.
Pretty good overall, though.

>You could randomize what you need in PlayGame(), save a function call and several lines of code.

It may not be about coding style but the above point is correct. You should not seed a random number generator in a frequently used function. A random number generator is coded to produce a large sequence of random numbers; it doesn't need a new seed every function call. Also as you've based the seed on time, your random number will only appear random if you call the functions less than once a second.

dont get too hung up on commenting every little thing. It is necesary for complex functions/algorithm but what you've shown us is pretty good and is farily self-explanatory thanks to the good variable names.

Too many comments can be a bad thing. I comment classes and functions (at the top of each) and anything that requires me to read it more than once after I have written it!!