Take a look at my program

This is a discussion on Take a look at my program within the C++ Programming forums, part of the General Programming Boards category; I wrote a simple Mastermind game. Please take a look at it and tell me your comments: Code: #include<iostream> #include<cstdlib> ...

  1. #1
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231

    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;
    }
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

  2. #2
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231
    So it seems that it is very good code with nothing wrong in it. Thank you.
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

  3. #3
    Registered User
    Join Date
    Jan 2005
    Posts
    7,317
    >> 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.

  4. #4
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231
    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.
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

  5. #5
    int x = *((int *) NULL); Cactus_Hugger's Avatar
    Join Date
    Jul 2003
    Location
    Banks of the River Styx
    Posts
    902
    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.
    long time; /* know C? */
    Unprecedented performance: Nothing ever ran this slow before.
    Any sufficiently advanced bug is indistinguishable from a feature.
    Real Programmers confuse Halloween and Christmas, because dec 25 == oct 31.
    The best way to accelerate an IBM is at 9.8 m/s/s.
    recursion (re - cur' - zhun) n. 1. (see recursion)

  6. #6
    Registered User
    Join Date
    Feb 2006
    Posts
    312
    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.

  7. #7
    int x = *((int *) NULL); Cactus_Hugger's Avatar
    Join Date
    Jul 2003
    Location
    Banks of the River Styx
    Posts
    902
    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. ;-)
    long time; /* know C? */
    Unprecedented performance: Nothing ever ran this slow before.
    Any sufficiently advanced bug is indistinguishable from a feature.
    Real Programmers confuse Halloween and Christmas, because dec 25 == oct 31.
    The best way to accelerate an IBM is at 9.8 m/s/s.
    recursion (re - cur' - zhun) n. 1. (see recursion)

  8. #8
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,484
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  9. #9
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231
    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;
    }
    Last edited by siavoshkc; 08-12-2006 at 04:23 PM.
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

  10. #10
    (?<!re)tired Mario F.'s Avatar
    Join Date
    May 2006
    Location
    Portugal
    Posts
    7,436
    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.
    Last edited by Mario F.; 08-12-2006 at 04:15 PM.
    The programmer’s wife tells him: “Run to the store and pick up a loaf of bread. If they have eggs, get a dozen.”
    The programmer comes home with 12 loaves of bread.


    Originally Posted by brewbuck:
    Reimplementing a large system in another language to get a 25% performance boost is nonsense. It would be cheaper to just get a computer which is 25% faster.

  11. #11
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231
    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.
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

  12. #12
    (?<!re)tired Mario F.'s Avatar
    Join Date
    May 2006
    Location
    Portugal
    Posts
    7,436
    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.
    The programmer’s wife tells him: “Run to the store and pick up a loaf of bread. If they have eggs, get a dozen.”
    The programmer comes home with 12 loaves of bread.


    Originally Posted by brewbuck:
    Reimplementing a large system in another language to get a 25% performance boost is nonsense. It would be cheaper to just get a computer which is 25% faster.

  13. #13
    System Novice siavoshkc's Avatar
    Join Date
    Jan 2006
    Location
    Tehran
    Posts
    1,231
    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.
    Learn C++ (C++ Books, C Books, FAQ, Forum Search)
    Code painter latest version on sourceforge DOWNLOAD NOW!
    Download FSB Data Integrity Tester.
    Siavosh K C

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Issue with program that's calling a function and has a loop
    By tigerfansince84 in forum C++ Programming
    Replies: 9
    Last Post: 11-12-2008, 12:38 PM
  2. Need help with a program, theres something in it for you
    By engstudent363 in forum C Programming
    Replies: 1
    Last Post: 02-29-2008, 12:41 PM
  3. Replies: 4
    Last Post: 02-21-2008, 09:39 AM
  4. My program, anyhelp
    By @licomb in forum C Programming
    Replies: 14
    Last Post: 08-14-2001, 10:04 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21