critique my code please

[Randal Johnson]

Here's a simple game I wrote, you think of a number and tell the computer
if its guesses are too high or too low or just right.

Anyways I would like someone with experience to tell me how good or bad my
program writing skills are. Is it easy to follow, could I have been more efficient, are my variables named well? Am I starting any bad habits? Anything really.

#include <iostream>
#include <cstdlib>
#include <time.h>
using namespace std;


int main() {
	char cLoop = 'Y';
	char cReady;
	int iReply;
	int iNumHi = 101;
	int iNumLo = 0;
	int iRandom;
	int iCompGuess;
	int iCompAttempts = 0;

while (cLoop == 'Y' || cLoop == 'y')
{

cout << "Please think of a number between 1 and 100.\nIf I guess the number in 7 tries or less, I win.\n Enter (Y) when ready.";
	cin >> cReady;

	iCompAttempts++;

	srand (time(NULL));
		iRandom = rand() % 100 + 1;
	// 1st step
		cout << "\nIs the number: " << iRandom << "?\n";
		cout << "\n(1)Correct. (2)Too High. (3)Too Low.\n";
		cin >> iReply;

		if (iReply == 1) { cout << "\n.\nWow, I'm good at this.\n"; }
		
		//too high
		else if (iReply == 2) { iNumHi = iRandom; 
					iCompGuess = (iNumHi + iNumLo) / 2;
							}
		//too low
		else if (iReply == 3) { iNumLo = iRandom; 
					iCompGuess = (iNumHi + iNumLo) / 2;
							}
		else { cout << "\nInvalid entry.\n"; }


			while (iReply != 1)
				{
				//2nd step
					iCompAttempts++;

							cout << "\nIs the number: " << iCompGuess << "?\n";
						cout << "\n(1)Correct. (2)Too High. (3)Too Low.\n";
						cin >> iReply;
				//While test
						if (iReply == 1) { cout << "\n.\nWow, I'm good at this.\n"; }
		
						//too high
						else if (iReply == 2) { iNumHi = iCompGuess; 
									iCompGuess = (iNumHi + iNumLo) / 2;
											}
						//too low
						else if (iReply == 3) { iNumLo = iCompGuess; 
									iCompGuess = (iNumHi + iNumLo) / 2;
									}
						else { cout << "\nInvalid entry.\n"; }

				}
		
			cout << "It took me " << iCompAttempts << " guesses.\n";
			
		cout << "\n\nWould you like to play again? (Y/N).\n";
		cin >> cLoop;
		iCompAttempts = 0;
}


return 0;
}

[nano511]

Make sure to keep your "rules" consitent. If your going to do this:

int function(){
}

then dont randomly switch to

int function()
{
}

Also, your spacing looks funky. Ill try to fix it and ill post the fixed version

E: Nvm i got lazy XD. But to make it easier to visualize each new "scope" you should always indent each new "body"( i don't know terms lol ).


Also, doing this is ugly:

if (iReply == 1) { cout << "\n.\nWow, I'm good at this.\n"; }

Looks much nice like this

if( iReply == 1 ) 
{ 
          cout << "\n.\nWow, I'm good at this.\n"; 
}

[ModShop]

Perhaps you should use a better number selection mechanism than rand(). Something that guesses the median of the current range would be good.

int min(0), max(100);
//adjust min/max based on user input (too high/low etc)

int guess = min + ((max-min)/2);

Also, you should check to make sure that the player is honest. If they say 25 is too high and 74 is too low in the same round, their full of it. The min/max range system would allow you to easily track this.

[Randal Johnson]

@nano511 Thanks man, that is exactly the type of feedback I was looking for.

I do like the example you gave

Looks much nicer like this

if( iReply == 1 ) 
{ 
          cout << "\n.\nWow, I'm good at this.\n"; 
}

I think this looks much cleaner too and this is how I tend to write my code. Do you ever try to make it more compressed so it isn't so long? or is it worth the sacrifice to make it more readable?

@ModShop, those are some good suggestion although I was looking more for tips on my... I guess you could call it 'grammar.' The program only uses rand() for the computers initial guess. After that it takes the average of the high and low numbers. Otherwise the game would be identical every time you picked the same number.

ps. Is my Hungarian notation being used correctly?

[shacktar]

Using some constants would be nice. It's also good to get into the habit of initializing all variables.

int main()
{
     const int LOWEST = 1;
     const int HIGHEST = 100;
     const int MAX_TRIES = 7;

     char cLoop = 'Y';
     char cReady = 0; //initial value
     int iReply = 0; //initial value
     int iNumHi = 101;
     int iNumLo = 0;
     int iRandom = 0; //initial value
     int iCompGuess = 0; //initial value
     int iCompAttempts = 0;

     while (cLoop == 'Y' || cLoop == 'y')
     {

          cout << "Please think of a number between " << LOWEST << " and " << HIGHEST << ".\nIf I guess the number in "
               << MAX_TRIES << " tries or less, I win.\n Enter (Y) when ready.";

          cin >> cReady;

          iCompAttempts++;

          srand (time(NULL));
          iRandom = rand() % (HIGHEST - LOWEST + 1) + LOWEST;

          ... ... ...

[TheMassiveChipmunk]

Use srand only once in a program.
http://www.cplusplus.com/forum/beginner/43680/

[krisitine lee]

don't forget to initialize the variables.

[ne555]

lines 27-42 are equal to lines 50-64. Avoid that repetition.

is it worth the sacrifice to make it more readable?

¿what sacrifice? That's IDE's work.

Is my Hungarian notation being used correctly?

I don't think that you should use Hungarian notation.
However, you may find interesting http://www.joelonsoftware.com/articles/Wrong.html

Also, you forgot to reset the limits (High, Low).
You better reducing the scope of that variables.

while( playing ){
  int attempts=0, low=LOWEST, high=HIGHEST; //they don't exist outside the loop 

[DSLq5Di1]

I personally find Hungarian Notation distracting and unnecessary, well named variables that state their purpose are of much more use.

Keep your variable declarations as local as possible. Ideally, declare a variable only when it is needed, and if possible, initialize with a meaningful value.

There is a lot of repetition, think about how you can minimize that.

[Moschops]

I personally find Hungarian Notation distracting and unnecessary, well named variables that state their purpose are of much more use.

Done correctly, that is what Hungarian Notation does. :)

[zb0S216C]

Here's my thoughts:

1) I dislike your use of white-space. The indentation is awful. Limit yourself to no more than a 6-space indent.

2) You have uninitialized variables. Once you've declared them, you cannot initialized them, only assign them a value (not initialization).

3) I dislike the use of using namespace std. It's lazy since your including the entire scope of the standard namespace, not the functions/classes you'll be using.

4) How're you supposed to see the results if you don't keep the console open? Wouldn't it be better if you manually wrote a global pause function? For example:

int Pause( const char *Message )
{
    if( Message != nullptr )
    {
        // Use the user-defined message instead of a default one.
        std::cout << Message << std::endl;
        std::cin.ignore( std::numeric_limits < std::streamsize >::max( ), '\n' );
        return 0;
    }

    // No message was given; provide a default one.
    std::cout << "Press any key to continue..." << std::endl;
    std::cin.ignore( std::numeric_limits < std::streamsize >::max( ), '\n' );
    return 0;
}

int main( )
{
    return Pause( "Waiting for you to press a key..." );
}

^Wazzak

[Moschops]

4) How're you supposed to see the results if you don't keep the console open?

Perhaps by running the program from a console and simply not closing it until the results have been read.

[zb0S216C]

Moschops wrote:

Perhaps by running the program from a console and simply not closing it until the results have been read.

I suppose you could, but that would mean opening command prompt, changing the working directory, and entering the specified executable name. Too much work if you ask me. Simply executing the program with a double-click is sufficient.

_Wazzak

[Randal Johnson]

4) How're you supposed to see the results if you don't keep the console open?

Or you could try running the program and see that I already created a while loop that asks you to enter Y/N at the end and it will either rerun the program or exit. Nicer than system pause.

Did anyone bother to paste this into their compiler before assuming they knew exactly how it worked?

@ne555

lines 27-42 are equal to lines 50-64. Avoid that repetition.

They aren't identical,
In 27-42 iNumHi/iNumLo = iRandom;
in lines 50-64 iNumHi/iNumLo = iCompGuess;
This way the initial guess is totally random, while the following guesses are all calculated.

Also, you forgot to reset the limits (High, Low).
You better reducing the scope of that variables.
while( playing ){
int attempts=0, low=LOWEST, high=HIGHEST; //they don't exist outside the loop

Thanks I totally missed that, and it completely messes up the game the second time around.


Could someone explain why it's important to initialize variables and assign a value to them at the same time?

[ne555]

//iRandom = rand() % 100 + 1;
CompGuess = rand() % 100 + 1;
//or
Random = rand() % 100 + 1;
CompGuess = Random;

The difference was artificial.