Sign in to follow this  
Sria

Finished a program challenge, would like some input please

Recommended Posts

Sria    157
So, if this is out of the norm and not an appropriate place for this just say so and I'll edit it right out! Anyway I decided to do a challenge out of the book that I got.. The challenge was: Write a program using vectors and iterators that allows a user to maintain a list of his or her favorite games. The program should be able to allow the user to list all titles, add a title, or remove a title. So I set out to do it with the best of my knowledge and this is what I ended up with. Any comments on it would be appreciated (ie point out where things could have been better or anything like that) I can take it so be honest. It's in C++ with Microsoft Visual Studio 2005
//Program to keep and maintain a list of favored games.
#include <iostream>
#include <vector>
#include <string>

using namespace std;

//function for adding a game to the list.
string addGames(string question);
//Function for removing a game from the list.
int removeGames(string question);
//Function for displaying the list
void listGames();



int main()
	{  //begin main()
	
	//Vector to store the list in.
	vector<string> gameList;
	//Iterator refering to objcts in gameList.
	vector<string>::iterator myGames;
	//Const int to refer and display the list.
	vector<string>::const_iterator listGames;
	string firstName;
	string quit = "no";

	//Welcoming the user
	cout<<"\t\t\tHello, and Welcome to Sria's game keeper!\n\n";
	cout<<"What's your name? ";
	cin>> firstName;
	cout<<"\n\nAlright "<< firstName <<", what would you like to do?\n\n\n";

	do    //Beggining main program loop.
	{
	
	//Stores the choice they make in the menu.
	int menuChoice;
	
	
	//Displaying the main menu
	cout<<"\n\n\n1 - Show current games in the list. \n";
	cout<<"2 - Add a game to the list. \n";
	cout<<"3 - Remove a game from the list. \n";
	cout<<"4 - Quit the program. \n";
	//End of menu

	cout<<"\n\nEnter your choice here - ";
	cin>> menuChoice;

//Trivial displaying of what the user chose :)
	switch(menuChoice)
		{
		case 1:
			system("cls");
			cout<<"\n\nBring on the list!";
			break;
		case 2:
			system("cls");
			cout<<"\n\nLets add to the list!";
			break;
		case 3:
			system("cls");
			cout<<"\n\nAlright, let's remove some games!\n\n";
			break;
		case 4:
			system("cls");
			cout<<"\n\nOk, but you're missing out on the most useful program of your lifetime!";
			break;
		default:
			system("cls");
			cout<<"\n\nSorry, but that is not a correct input choice.";
			break;
		}
//End switch statement

	
	

	//If they chose show list...
	
	if (menuChoice == 1)
		{
		int gameNum = 0;
	    cout<<"\n\nYour game list is: \n\n";
	
	for (listGames = gameList.begin(); listGames != gameList.end(); ++listGames)
		{
		++gameNum;
		cout<< gameNum <<" - "<< *listGames << endl;
		}
		}
	//End Show list option

	//If they Chose add game option.
	if (menuChoice == 2)
		{
		char again = 'y';
		do{

			
		string addGame = addGames("\nEnter a new game for your list! \n");
		gameList.push_back(addGame);
		cout<<"\nWould you like to add another game? <y/n> " << endl;
			
		cin>>again;
			}while (again != 'n');
		}

	//End Add game option

	//If they chose remove game option.
	if (menuChoice == 3)
		{
		char removAgain = 'y';
		do
			{
			cout<<"Your current game list is : \n\n";
		int b = 0;
		for (listGames = gameList.begin(); listGames != gameList.end(); ++listGames)
			{
			++b;
			cout<< b << " - " << *listGames << endl;
			}
			
			int remove = removeGames("\nType the number which corresponds with the game you want to remove to have it taken off your list. ");
		gameList.erase((gameList.begin() + remove) - 1);
		

		cout<<"Your updated game list is: \n\n";
		int a = 0;
		for (listGames = gameList.begin(); listGames != gameList.end(); ++listGames)
			{
			++a;
			cout<< a << " - " << *listGames << endl;
			}
		cout<<"\n\nWould you like to remove another game? <y/n> ";
		cin>>removAgain;
			}while (removAgain != 'n');

		}
	//End remove game option.

	//If they chose quit
	if (menuChoice == 4)
		quit = "quit";
	//end quit option

	}while (quit != "quit");

	cout<<"\n\n";
	system("PAUSE");
	return 0;
	}

inline string addGames(string question)
	{
	

	string answer;
	cout<< question <<endl;
	cin>> answer;
	return answer;

	}
int removeGames(string question)
	{
	int answer;
	cout<< question << endl;
	cin>> answer;

	return answer;
	}


Anyway like I said, if critiquing programs is not something people do (i love art and am used to always having someone more educated/talented critiquing my work) I'll just remove it and we can forget it ever happend ;) But this was really my first project that I went totally solo on and didn't customize examples from the book so I am pleased :) Thanks

Share this post


Link to post
Share on other sites
v0dKA    568
I didn't look through all of it yet, but here are a couple criticisms already:

1. Don't do using namespace std;. It's considered bad practice. Instead, qualify everything that belongs in the std namespace using std::. This isn't a big concern for small programs, but it is when your program grows to a considerable size with a large number of identifiers. It's best to set good habits now.

2. I can't help but notice how you use system("cls"); in your switch block. Take a closer look: that statement is located in every control path inside that block: that is, no matter which choice the user picks, this statement will be called. Again, not a huge deal, but you should learn to notice these things - instead of duplicating that statement for every control path, simply write it once before you get to the switch block. I think of it as pulling out the common factor as in a math expression, but then again, I'm weird [wink]

Share this post


Link to post
Share on other sites
chollida1    532
Quote:
Original post by v0dKA
1. Don't do using namespace std;. It's considered bad practice. Instead, qualify everything that belongs in the std namespace using std::. This isn't a big concern for small programs, but it is when your program grows to a considerable size with a large number of identifiers. It's best to set good habits now.


I'd modify this to say don't use using namespace std; in header files, its considered bad practice. Doing this in a cpp file is not only considered ok, its the reason the using namespace xxx was added to the language.

Cheers
Chris

Share this post


Link to post
Share on other sites
Sria    157
I have only heard about header files and have not started programming yet, but I will keep the namespace thing in mind, it probably couldn't hurt because then I would know what actually does belong to the std, because right now I honestly don't.

Thank you very much for your crits though :)

Share this post


Link to post
Share on other sites
Sria    157
That is probably a good idea, if i simply put it as say.

void listGame()
{

for (listGame = gameList.begin(); etc etc,

}

then called it in main, would that work or is there something else I'd have to know.

Share this post


Link to post
Share on other sites
v0dKA    568
Many games have spaces in their names. Using just std::cin, you have no way to account for such names (try typing in something with a space under the "Add game" option and see what happens). You'd be better off using std::cin.getline() for that.

Fix your addGames() function to look more like the following:


inline string addGames(string question)
{


char answer[ 1024 ];
cout<< question <<endl;
cin.getline( answer, 1023 );
return string(answer);

}


Which brings up a question of my own: under std::cin.getline(), are you supposed to use 1023 or 1024 for the second parameter given a string of length 1024?

Another criticism: in order to force output on the next line, you are mixing \n and endl. Stick to one or the other. You're asking for trouble if you mix both of them.

EDIT: a semi-criticism: you name the function addGames(), when in fact you use the function to add only one game. It really quite bothers me. Fix it for my sake, please [wink]

Share this post


Link to post
Share on other sites
rip-off    10979
Quote:
Original post by v0dKA
Many games have spaces in their names. Using just std::cin, you have no way to account for such names (try typing in something with a space under the "Add game" option and see what happens). You'd be better off using std::cin.getline() for that.

Fix your addGames() function to look more like the following:


inline string addGames(string question)
{


char answer[ 1024 ];
cout<< question <<endl;
cin.getline( answer, 1023 );
return string(answer);

}


Which brings up a question of my own: under std::cin.getline(), are you supposed to use 1023 or 1024 for the second parameter given a string of length 1024?


I believe you use:

string addGames(string question)
{
string answer;
cout << question << endl;
getline( cin, answer );
return answer;
}


[grin]

Share this post


Link to post
Share on other sites
RAZORUNREAL    567
Quote:
Original post by Sria
That is probably a good idea, if i simply put it as say.

void listGame()
{

for (listGame = gameList.begin(); etc etc,

}

then called it in main, would that work or is there something else I'd have to know.


No, it wouldn't find gameList because that's in main. You have to pass in the list somehow. There are a couple of ways. Here's a quick and dirty one:


void listGames(const vector<string>& gameList)
{
for (listGame = gameList.begin(); etc etc, as you said
}

//To call it:
listGames(gameList);


Kind of funny how gameList isn't a list. Oh well. Your doing well btw.

Share this post


Link to post
Share on other sites
Sria    157
Hehe, thank you for the crits. I will work on using the getline (cin, answer) thing. I will attempt to make a function for listing the games.


And about the names I used for things well..

addGames - in my mind i was thinking, this will be the function that I use to addGame(s) to my list ;P

and I don't understand why gameList isn't a list. Don't get me wrong I believe you I just don't understand why. In my mind it was where all games went in to then you show everything in gameList so wouldn't that be the list.

Well anyway I will get to work on updating that. I don't think I'll bore you guys with posting all the source again but I may need help with the new cin commands :O


Thanks again

Share this post


Link to post
Share on other sites
wild_pointer    291
Just some really general stuff:

You have a bit too much going on in main, there shouldnt be much actual work being done there.

You have a big switch statement thats only purpose is to print a message, why not move all the logic in there (move it into functions first, while you're at it) instead of rechecking menuchoice later?

Declare variables near first use, not all at once at the beginning of a code block (this isn't C89).

Don't use inline. If you understand what it actually means to declare something inline well enough to prove that my previous statement isn't always good advice, then you can use it =)

Please don't let RAZORUNREAL confuse you as far as gameList not being a list goes =P You're correct in your thinking.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this