Messy bunch o' code

[Akkernight]

Well, I made this simple program, which is not complete yet, but any suggestions of improving code would be great
Anyways, I'm mostly posting here 'cause I need to transfer the code, now that I don't have access to my home PC :/ oh and I do know that the code should be seperated into files, but I'm too lazy to do that :P

main.cpp

#include <iostream>
#include "product.h"

using std::cout;	using std::endl;
using std::vector;	using std::string;
using std::cin;

void addProd( const string name, const double price, const categories category,
			 vector<product>& vec);
void addGatherInfo(vector<product>& prodVec);
string categoryToString(const categories& check);
void prodRemove(string name, vector<product>& vec);

int main(){
	bool run = true;
	vector<product> prodVec;
	while(run == true){

	string input;
	cout<< "\nWant to add a new product? Type: add" << endl;
	cout<< "Want to see a list of added products? Type: list" << endl;
	cout<< "Want to remove a product? Type: remove" << endl;
	cout<< "Want to exit the program? Type: exit" << endl;
	cin>> input;
	if(input == "add"){
		addGatherInfo(prodVec);
	}

	if(input == "list"){
		for(vector<product>::size_type i = 0; i != prodVec.size(); ++i){

			cout<< "\nProduct name: " << prodVec[i].name;
			cout<< "\nProduct price: " << prodVec[i].price;
			cout<< "\nProduct category: " << categoryToString(prodVec[i].category);
			cout<< "\n----------------\n";
		}
	}

	if(input == "remove"){
		string name;
		cout<< "Please type in a name for the product: ";
		cin>>name;
		prodRemove(name, prodVec);
	}

	

	if(input == "exit"){
		run = false;
	}	
	
	}
	return 0;
}

void addProd( const string name, const double price, const categories category,
						vector<product>& vec) {

				product prod;
				prod.name = name;
				prod.price = price;
				prod.category = category;

				vec.push_back(prod);

}

void addGatherInfo(vector<product>& inpVec){

		string name;
		double price;
		categories category;

		cout<< "Please type in a name for the product: ";
		cin>> name;
		cout<< "\nPlease type in a price for the product: ";
		cin>> price;
		cout<< "\nPlease choose a category for the product: " <<endl;
		cout<< "1. vegetables\n" << "2. drink\n" << "3. meat\n";
		cout<< "Type in one of the numbers for those categories: ";
		int choNum;
		cin>> choNum;
		switch(choNum){
			case 1:
				category.vegetable = true;
				break;
			case 2:
				category.drink = true;
				break;
			case 3:
				category.meat = true;
				break;
		}
	
		addProd(name, price, category, inpVec);
}

string categoryToString(const categories& check){
	if(check.vegetable == true)
		return "Vegetable";
	if(check.drink == true)
		return "Drink";
	if(check.meat == true)
		return "Meat";

	return "Product category not defined!";
}

void prodRemove(string name, vector<product>& vec){
	for(vector<product>::size_type i = 0; i != vec.size(); ++i){
		if(vec[i].name == name){
			vec.erase(vec.begin() + i);
			break;
		}
	}
}

main.h

#ifndef MAIN_H
#define MAIN_H

#include <string>
#include <vector>

struct categories{
	bool vegetable;
	bool drink;
	bool meat;
};

/*struct product {
	std::string name;
	double price;
	categories category;
};*/

#endif

product.cpp

#include "product.h"

using std::string;

product::product(){
	category.vegetable = false;
	category.drink = false;
	category.meat = false;
}

product::~product(){
}

product.h

#ifndef PRODUCT_H
#define PRODUCT_H

#include "main.h"

class product
{
public:

	product();
	~product();

	std::string name;
	double price;
	categories category;

protected:


};

#endif

[matsp]

struct categories{
	bool vegetable;
	bool drink;
	bool meat;
};

If you never want vegetable, drink and meat to be set at the same time, then an enum is probably a better choice. [Your current output doesn't appear to deal with more than one set right now anyways].

	if(input == "/add"){

(And the others like it): Why do you have a slash at the front of the name of the command - seems completely arbitrary and unnecessary.

--
Mats

[Akkernight]

It's just what I'm used to seeing from games(And looks more clean by my eyes) :P and I've never learnt about enum :/

[matsp]

Sorry, hit submit before I meant to, so posting a second post.

vector<product> addProd( const string name, const double price, const categories category,
						vector<product>& vec) {

				product prod;
				prod.name = name;
				prod.price = price;
				prod.category = category;

				vec.push_back(prod);

				return vec;
}

Why pass in a reference to the vector, and then return it. The latter copies the entire vector [and as you don't use the returned value at all, it is a complete waste of time].

		for(vector<product>::size_type i = 0; i != prodVec.size(); ++i){

			cout<< "\nProduct name: " << prodVec[i].name;
			cout<< "\nProduct price: " << prodVec[i].price;
			cout<< "\nProduct category: " << checkCategory(prodVec[i].category);
			cout<< "\n----------------\n";
		}

This should probably be a function. The name "checkCategory" would be better called "categoryToString" or something like that.

--
Mats

[Akkernight]

Well the reason I used referance is 'cause I thought that was the right thing for that use xD Thought I was being clever 8-)

[matsp]

Well the reason I used referance is 'cause I thought that was the right thing for that use xD Thought I was being clever 8-)

Yes, that IS clever. Returning a copy of the vector is not.

--
Mats

[matsp]

It's just what I'm used to seeing from games(And looks more clean by my eyes) :P and I've never learnt about enum :/

Time to start learning about enums then.

Using a "simple" user-interface is a good idea. Adding extra characters for some arbitrary reason is not good user-interface design.

--
Mats

[Akkernight]

Oh yeah! I'm calling main() in main(), making it recursive, this was just something I did, 'cause I had no other idea on how to not shut the program each time it ran, but is this wise to do?

[matsp]

Oh yeah! I'm calling main() in main(), making it recursive, this was just something I did, 'cause I had no other idea on how to not shut the program each time it ran, but is this wise to do?

Absolutely NOT the right thing to do. I missed that. Just use some form of loop (for, while, do-while) - since the call to main() is unconditional, just doing an while(true) or for(; will do what you want.

--
Mats

[hk_mp5kpdw]

void prodRemove(string name){
	for(vector<product>::size_type i = 0; i != prodVec.size(); ++i){
		if(prodVec[i].name == name){
			prodVec.erase(prodVec.begin() + i);
			break;
		}
	}
}

From the other post related to the prodRemove function you should be using < and not !=.

Also, you should avoid using globals as much as possible so you should probably be passing in a vector to the function (you are passing in one to other functions [addGatherInfo/addProd] but not this one for some reason).

[Akkernight]

Well, I didn't make addProd vector thing a reference and it didn't work anymore :P

And I told you I fixed it xD Just by making the vector global :P I'll fix that...

[matsp]

Well, I didn't make addProd vector thing a reference and it didn't work anymore :P

And I told you I fixed it xD Just by making the vector global :P I'll fix that...

No, no, no! Do not EVER fix a problem by making a variable global [Ok, so once in a while that may well be the right thing, but that is VERY rare].

You should use a reference in addProd [a reference means that you pass the original vector rather than a copy, so it's exactly the right thing to do here, since you want to change the content of the vector].

It is the "return vec" that is wrong - make it a void function, and simply do not return anything, and all will be fine.

--
Mats

[Akkernight]

Ohh... I see xD Thanks!

Next thing will be files... That was the ostream library, right?

[laserlight]

Speaking of call by reference, you should be passing references to string and the other struct as well. Making the price parameter const is only useful to you as the implementer of the function. It makes no difference to the caller whether price is const or non-const since the argument is passed by value.

Later on should product become a full blown class addProd would then be a member function.

[matsp]

Later on should product become a full blown class addProd would then be a member function.

Surely you mean that addProd would be the member of an object that holds the vector of products?

--
Mats