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.

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

---

Quote:

---

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 still dont know whether <iostream> or <iostream.h> is better though
<iostream> is the current standard, so it's better ;)

>My problem is I don't comment enough, and I hard code things too often.
A good rule of thumb for commenting is if it's doubtful, it's dirty. If you have any doubt that the code is self commenting, add a comment.

The code is okay I guess, I don't see anything terribly wrong. I don't like that much whitespace, but that's just me. Though if you have a bunch of functions or large functions in the program it is usually a good idea to prototype them and place them after main. This way readers can follow the program flow more easily.

-Prelude

dont make those lines:
//----------------------------------------------------------------
I once used to do that but I mean for me now it makes it harder to read

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.

Read the book "Code Complete." Surprisingly it's a MS book but there's a great deal of information that's useful, most of which is common sense, but some of it you just don't think about. It's getting a bit dated but is still useful in my opinion.

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!!