Take a look at my program

I wrote a simple Mastermind game. Please take a look at it and tell me your comments:

Code:

#include<iostream>
#include<cstdlib>
#include<iomanip>
#include<ctime>
#include<conio.h>
#include<ctype.h>
using namespace std;

#define PADLEN 14   //For aligning output
#define CHARNUM 3   //Number of characters to be guessed
//Initializes screen and makes a random generated secret word
void intitsecret(char s[CHARNUM]){
	cout<<endl<<"Guess"<<setw(PADLEN)<<"Correct"<<setw(PADLEN)<<"Guess#"<<setw(PADLEN)<<"Misplaced"<<endl;
	tm *tmst;
	time_t t=time(NULL);
	tmst = gmtime(&t);
	srand(tmst->tm_sec);
	for(int i = 0; i<CHARNUM;i++){
		char rnd =(rand()%26)+97;
		s[i] = rnd;
	}
}
//Gets a guess word from player
bool getguess(char g[CHARNUM]){

	for(int i =0;i<CHARNUM;i++){
		char t= (char) _getch();
		if(t==27) return true; //Escape to exit program
		//Making back space functional
		if(t==8 && i>0){_putch(8);i--;_putch(' ');_putch(8);} 
		if((t>0)&& isalpha(t)&& !isdigit(t)){
			t=tolower(t);
			_putch(t);
			g[i]=t;
		}
		else i--;
	}
	return false;
}
//Compares guessed work with secret word and shows the result.
//In addition it returns true if the guessed and secret words matched.
bool comp_show(char s[CHARNUM],char g[CHARNUM]){ 
	
	static int count;
	count++;
	int cp=0;
	int mp=0;
	char t[CHARNUM];
	strncpy(t,s,CHARNUM);
	for(int i=0 ;i<CHARNUM;i++)
		if(t[i]==g[i]){
			t[i]='\0';
			cp++;
		}
	
	for(int i=0 ;i<CHARNUM;i++)
		for(int j=0;j<CHARNUM;j++)
			if(t[i]==g[j]){
				t[i]='\0';
				mp++;
			}	

	cout<<setw(PADLEN)<<cp<<setw(PADLEN)<<count<<setw(PADLEN)<<mp<<endl;
	if(cp==CHARNUM) return true;
	else return false;

}
int main(){
	char g[CHARNUM+1];
	char s[CHARNUM];
	intitsecret(s);

	for(int i=0;i<40;i++){
		if(getguess(g)) break;
		if(comp_show(s,g)){ //Player won
			cout<<"Correct! ";
			
			if(i>10)
				cout<<"Very good, you are very smart."<<endl;
			else if (10<i && i<30)
				cout<<"Not bad."<<endl;
			else
				cout<<"Not good, you should try to guess faster."<<endl;

			_getch();	
			exit(0);
		}
	}
	//Player lost or escaped
	cout<<endl<<"You lost! The word was: ";
	for(int i=0 ;i<CHARNUM;i++) _putch(s[i]);
	_getch();
	return 0;
}

So it seems that it is very good code with nothing wrong in it. Thank you.

>> So it seems that it is very good code with nothing wrong in it.
I wouldn't assume that just because you haven't got a response. I can see a few things at a quick glance that I would change, like non-standard code and formatting issues, but whether or not there are any bugs or bad code would take a closer inspection.

So it seems that it is very good code with nothing wrong in it.

Annoying words to make people say something about the code ;-).

wouldn't assume that just because you haven't got a response. I can see a few things at a quick glance that I would change, like non-standard code and formatting issues, but whether or not there are any bugs or bad code would take a closer inspection.

Please tell me about non-standard code and formatting issues.

Code:

mm.cpp:5:18: error: conio.h: No such file or directory
mm.cpp: In function 'bool getguess(char*)':
mm.cpp:27: error: '_getch' was not declared in this scope
mm.cpp:30: error: '_putch' was not declared in this scope
mm.cpp:33: error: '_putch' was not declared in this scope
mm.cpp: In function 'int main()':
mm.cpp:85: error: '_getch' was not declared in this scope
mm.cpp:91: error: '_putch' was not declared in this scope
mm.cpp:92: error: '_getch' was not declared in this scope

That's the nonstandard part. In a nutshell: conio.h isn't available on every system - very few, in fact. I generally recommend curses over conio - I've yet to meet a platform that didn't have some form of curses. Compiles in Windows fine, but fails elsewhere.

You also make some assumptions about what the binary values of characters are. I've yet to see a system differ from the norm (what you used), but it could happen. Aside, it makes your code clearer and easier to read if we're not looking at the numerical values of letters. Take:

Code:

char rnd =(rand()%26)+97

(rnd is never used here, except on the next line where it's assigned to the array... seems you could just leave it out entirely.) What is 97 in that line? It's easier to figure out if you know what the program does: it's the numerical value of the lowercase A. So, since that's what it is, write it as such:

Code:

char rnd = (rand() % 26) + 'a'

Now your code only assumes that the letters are in order, but 'a' could be any value. More portable, and easier to read. (The parenthesis are unneeded, but sometimes they increase readability.)

As for style, that's a personal thing. For me, however: I perfer that the opening brace for code blocks is on a newline, aligned with whatever opened it. I like a space between my operators and their operands. (ie, "1 + 2", not "1+2"). That's just preference however.
Also, I'd give your variables twee more descriptive names.

Good and bad are all relative to the stage of your learning and what you're trying to do IMO

For a novice program, i'd say it's not terrible. but there are certainly many ways you could improve

Firstly, don't use ctype.h in C++.. the correct standard header is <cctype>

Code:

#define PADLEN 14   //For aligning output
#define CHARNUM 3   //Number of characters to be guessed

This is very c-esque, looking at your code, I assume you've come from a C background (or are learning from a course/book/tutor who teaches "A better C") . in C++, this is bad style. Firstly, you should strive to use const to create constants instead of #define ... and secondly, don't make global variables unless you really have to. #define has no type checking at all, and for this reason, can be quite error prone. const variables, on the other hand, are type-checked just like any other variable. Global variables are nearly always synonymous with bad design. You can find yourself modifying that variable all over the place, without any safeguards, and quickly lose track of where the variable is modified.

Once you start getting bugs in even small-medium sized programs,, you'll see how bugs involving global variables are pretty awkward to track down. it also consumes a name, which you might reuse somewhere else, or get a clash with another name somewhere. Quite often, there are good design decisions which you can take to avoid global variables altogether.

Since you're in C++, you should get to grips with the <string> library at your earliest convenience.. I reckon you could simplify that program alot just by using a few of the nice features which come supplied with the string library (Like the fact that you don't need to specify the length of the string... and overloaded comparison operators)

final comment, you use alot of literal numbers in your program. Its not a good habit to get into. If you need constants, give them meaningful names, because then whoever's reading your code (Or yourself in 6 months time when you come to revisit it) won't be sitting there asking him/her self "What's so special about that magic number there?" ... self-documenting code also eliminates alot of /* */ and // comments ... remember, high level programming languages are designed to bring computer code more in line with natural human language.

Bug. These conditions probably aren't what you meant, so check your logic:

Code:

			if(i>10)
				cout<<"Very good, you are very smart."<<endl;
			else if (10<i && i<30)
				cout<<"Not bad."<<endl;
			else
				cout<<"Not good, you should try to guess faster."<<endl;

That middle condition can prove true, but will never get the chance. For my 15 guess game, I got "Very good".

Otherwise, seems very solid. Nice job. (Now, can you make it only generate real words? ;-) )
EDIT: And don't just flip the sign on the top on to i<10 - that isn't right either. ;-)

> srand(tmst->tm_sec);
Rather than having a different seed each time with srand(time(NULL)), you now only have 60 possible seed values.

It wouldn't take long to figure out that the start time of the program was related to the generated secret.

First, thanks for advices.

About <ctype>, it was a typing mistake, sorry.

I corrected time.

I used <string> in this edition, but I think it could be used in a better way, no?

I am currently learning from a book named "A Beginners' C++", that is really a good book. Its teaching method is very good. It teaches how to think about coding and how to design programs. Unfortunately it has a bad style. It uses <iostream.h> for example, and some of its codes are considered wrong:

Code:

for(int i = 0 ; i < n ; i++) {…}
for(i = 33 ; i < m ; i++) {…}

It works fine in VC++6, but is non standard. It also says you can't make externs const.

I downloaded this book from the web. I also downloaded Bruce Eckel book and I want to read it after this one. Farsi translations of computer books are completely unreadable because of very bad translation (at least I can't understand them). So I am limited to free English books in the web.

Anyway, what should I do with _getche()? How can I make it portable? I don't know any method to the same job.
I included .exe file for those who can't compile it.
This is the new edition:

Code:

#include<iostream>
#include<cctype>
#include<string>
#include<iomanip>
#include<ctime>
#include<conio.h>

using namespace std;

const char ESC_KEY = 27;
const char BS_KEY = 8;
//Initializes screen and makes a random generated secret word
void initSecretWord(string &secretWord, const int CHARNUM){	
	srand(time(NULL));
	secretWord.resize(CHARNUM);
	for(int i = 0; i<CHARNUM;i++){
		char rnd =(char)(rand()%26) + 'a';
		secretWord[i] = rnd;
	}
}
//Gets a guess word from player
bool getUserGuess(string &guessedWord, const int CHARNUM){
	guessedWord.resize(CHARNUM);
	for(int curserPos = 0 ; curserPos < CHARNUM; curserPos++){
		char tempGuessChar =(char) _getch(); //What should I do with this?
		if(tempGuessChar == ESC_KEY) return true; //Escape to exit program
		//Making back-space functional
		if(tempGuessChar == BS_KEY && curserPos>0){
			curserPos--;
			cout.put(BS_KEY);
			cout.put(' ');
			cout.put(BS_KEY);
		} 
		if((tempGuessChar > 0)&& isalpha(tempGuessChar)&& !isdigit(tempGuessChar)){
			tempGuessChar = (char)tolower(tempGuessChar);
			cout.put(tempGuessChar);
			guessedWord[curserPos] = tempGuessChar;
		}
		else curserPos--;
	}
	return false;
}
//Compares guessed work with secret word.
//In addition it returns true if the guessed and secret words matched.
int compare(string &secretWord, string &guessedWord,  int &missPlaced, const int CHARNUM){ 
	int correctPlace = 0;
	missPlaced = 0;
	string tempSecretWord;
	tempSecretWord=secretWord;
	for(int i = 0 ;i<CHARNUM;i++)
		if(tempSecretWord[i] == guessedWord[i]){
			tempSecretWord[i] = '\0';
			correctPlace++;
		}
	for(int i=0 ;i<CHARNUM;i++)
		for(int j=0;j<CHARNUM;j++)
			if(tempSecretWord[i] == guessedWord[j]){
				tempSecretWord[i] = '\0';
				missPlaced++;
			}	
	return correctPlace;
}
int playGame(string &secretWord, const int PADLEN, const int GAME_ROUNDS, const int CHARNUM){
	cout.fill('-');
	cout<<"Press ESC to see the secret combination and exit."<<endl
		<<endl<<"Guess"<<setw(PADLEN)<<"Correct"<<setw(PADLEN)<<"Guess#"
		<<setw(PADLEN)<<"Misplaced"<<endl;
	string guessWord;
	int correctPlace, missPlaced;
	
	for(int round=1 ; round <= GAME_ROUNDS ; round++){
		if(getUserGuess(guessWord, CHARNUM)) break;
		correctPlace = compare(secretWord, guessWord, missPlaced, CHARNUM);
		if( correctPlace == CHARNUM){ //If true, player won
			
			return round;
		}
		cout<<setw(PADLEN-2)<<correctPlace<<setw(PADLEN)<<round
		     <<setw(PADLEN)<<missPlaced<<endl;
	}
	//Player lost or escaped!
	return 0;
}
int main(){
	const int gPADLEN = 14;   //For aligning output
	const int gCHARNUM = 3;   //Number of characters to be guessed
	const int gGAME_ROUNDS = 40;
	string secretWord;
	initSecretWord(secretWord, gCHARNUM);
	int round = playGame(secretWord, gPADLEN, gGAME_ROUNDS, gCHARNUM);
	if( round ){
		cout<<endl<<"Correct! ";
		const int SCORE = gGAME_ROUNDS / 3;
		if(round < SCORE)
			cout<<"Very good, you are very smart."<<endl;
		else if ((round >= SCORE) && (round <(SCORE * 2)))
			cout<<"Not bad."<<endl;
		else
			cout<<"Not good, you should try to guess faster."<<endl;
	}
	else
		cout<<endl<<"You lost! The word was: "<<secretWord;
			
	cout<<endl<<"Press a key to exit...";
	_getch();
	return 0;
}

Code:

for(int i = 0 ; i < n ; i++) {…}
for(i = 33 ; i < m ; i++) {…}

I'm not following. What's non standard about that?
The only (mildly) ugly thing I see in it is perhaps the use of the postfix increment. But even so, that is really not a problem.

I'm not following. What's non standard about that?
The only (mildly) ugly thing I see in it is perhaps the use of the suffix incremet. But even so, that is really not a problem.

The variable i is not redefined in the second loop.

Hmm... just noticed you are using windows 2005. Thats what was confusing me because you said it was working for you, so I was assuming "i" was being declared somewhere.

You may want to chech your compiler settings. I don't recall the exact name, but you have under C++ compiler options, an option to disable that behavior. It is working with you because your compiler is set to be backwards compatible with VC++ 6.0 behavior in which for loops didn't introduce a new scope.

Maybe I explained it in a bad way. First I started with VC++6. So that code worked fine. Now I am using VC++2005 which that code doesn't work in it.