My Code: Organized or Hell in Ascii??

6,963

Author

October 20, 2002 07:28 AM

hey! i'm writing a program that writes allows you to manipulate employees (50 of them) in, at most, 20 companies. I plan to later use an array because at this point I have no better solution. Well I just wanted anyone to look at my code and tell me if I'm on the right path and is there anyway to make it better. NOTE: no i'm not asking for help on my homework!! EDIT: removed for source ... current version below (work in progress) Any constructive suggestions and comments will be very helpful. I'm making sure I'm heading in the right direction and my code is clear enough (that way I won't have to put that many comments ). I do have one question though. If I have a list (array) and I ask the user to enter a employee and/or company name so I can search the list for that person, would it be better to do that search within the class functions or write another function (which will be main () ) that does that for me? note: this is definitely not a complete product. just preliminary. [edited by - Alpha_ProgDes on October 20, 2002 10:59:34 AM] [edited by - Alpha_ProgDes on October 20, 2002 11:04:39 AM] [edited by - Alpha_ProgDes on October 20, 2002 12:42:03 PM] [edited by - Alpha_ProgDes on October 20, 2002 12:46:59 PM] this is the revised version (sorry no comments). right now while running the debugger one of my variable (EmpName) is not getting on the stack. i have no idea why. so if you know, let me know. otherwise i'll be tackling this now. and thanks Arlid Fines. //employee h.

  
#ifndef ASS_2_H
#define ASS_2_H

#include <string>

class _employee
{
public:
	_employee ();
	_employee (std::string name);
	void setEmployee (std::string name);
	int calculateSalary (enum EmployeeStatus);
	bool searchName(std::string name);
	void printEmployeeNames();

private:
	std::string name;
	int rank;
};

#endif

//employee cpp.

        
#include <iostream>
#include <string>
#include "Ass_2.h"

//#ifndef EMPLOYEESTATUS

//#define EMPLOYEESTATUS
extern enum EmployeeStatus {PROMOTE, UNEMPLOYED, DEMOTE} status;

//#endif


_employee::_employee ()
{
	name = "";
	rank = 1;
}

_employee::_employee (std::string e_name)
{
	name = e_name;
	rank = 1;
}

void _employee::setEmployee (std::string setEmpName)
{

	name = setEmpName;
	rank = 1;
}

int _employee::calculateSalary (EmployeeStatus status) 
{
	const int PAYRATE = 1000;
	static int employeeSalary = 0;

	if (status == PROMOTE) {
		rank += 1;
		employeeSalary = rank * PAYRATE;
	}
	else if (status == DEMOTE) {
		rank -= 1;
		employeeSalary = rank * PAYRATE;
	}
	else { 
		rank = 0;
		employeeSalary += 50;
	}
	
	return employeeSalary;
}
	
	
bool _employee::searchName(std::string employeeName)
{
	if (name == employeeName)

		return true;
	else 
		return false;
}

void _employee::printEmployeeNames()
{
	std::cout << name << std::endl;
}

//company h.

        
#ifndef COMPANY_H
#define COMPANY_H

#include <string>
#include "Ass_2.h"

const int MAX = 50;

class _company
{
public:
	_company::_company();
	_company::_company(std::string empName);
	void setCompanyName(std::string compName);
	bool isCompanyFull();
	bool isCompanyEmpty();
	void join_Company (std::string empName);
	void promote_Employee (std::string empName);
	void demote_Employee (std::string empName);
	void change_Employee (std::string empName, std::string compName, _company& anotherCompany);
	void Employee_quits (std::string empName, _company& unemployed);
	void printCompanyNames ();
private:
	int total_employee;
	_employee Employee[MAX];
	std::string name;
	
};

#endif

//company cpp

        
#include <iostream>
#include <string>
#include "Ass_2.h"
#include "company.h"

enum EmployeeStatus {PROMOTE, UNEMPLOYED, DEMOTE} status;

_company::_company()
{
	name = "";
	total_employee = 0;
}

_company::_company (std::string compName)
{
	name = compName;
	total_employee = 0;
}

void _company::setCompanyName (std::string compName)
{
	name = compName;
	total_employee = 0;
}

bool _company::isCompanyFull()
{
	
	if (total_employee == MAX)
		return true;
	else
		return false;
}

bool _company::isCompanyEmpty()
{
	
	if (total_employee == 0)
		return true;
	else
		return false;
}

void _company::join_Company(std::string empName)
{
	if (isCompanyFull() == true)
		std::cout << "We are not hiring. Find another job!" << std::endl;
	else if (isCompanyEmpty() == true)
		Employee[0].setEmployee(empName);
	else {
		for (int empIndex = total_employee - 1; empIndex < 0; empIndex--){
			Employee[empIndex + 1] = Employee[empIndex];
		}
		Employee[0].setEmployee(empName);
	}
	total_employee++;
}

void _company::promote_Employee(std::string empName)
{
	int empIndex = 0;
	for (empIndex = total_employee - 2; empIndex <= 0; empIndex--)
		Employee[empIndex].searchName(empName);
	status = PROMOTE;
	Employee[empIndex].calculateSalary(status);

	_employee temp = Employee[empIndex];
	Employee[empIndex] = Employee[empIndex + 1];
	Employee[empIndex + 1] = temp;

}

void _company::demote_Employee(std::string empName)
{
	int empIndex = 0;
	for (empIndex = total_employee - 1; empIndex < 0; empIndex--)
		Employee[empIndex].searchName(empName);
	status = DEMOTE;
	Employee[empIndex].calculateSalary(status);

	_employee temp = Employee[empIndex];
	Employee[empIndex] = Employee[empIndex - 1];
	Employee[empIndex - 1] = temp;
}

void _company::change_Employee(std::string empName, std::string compName, _company& anotherCompany)
{
	int empIndex = 0;
	int index = 0;
	for (empIndex = total_employee - 1; empIndex < 0; empIndex--)
	{
		Employee[empIndex].searchName(empName);
		if (Employee[empIndex].searchName(empName) == true)
			break;
	}
	for (index = empIndex - 1; index <= 0; index--)
		Employee[index] = Employee[index - 1];

	anotherCompany.name = compName;
	anotherCompany.join_Company(empName);
}

void _company::Employee_quits(std::string empName, _company& unemployed)
{
	
	int empIndex = 0, index = 0;
	unemployed.join_Company(empName);
	
	for (empIndex = total_employee - 1; empIndex <= 0; empIndex--){
		Employee[empIndex].searchName(empName);
		if (Employee[empIndex].searchName(empName) == true)
			break;
	}
	status = UNEMPLOYED;
	Employee[empIndex].calculateSalary(status);

	for (index = empIndex - 1; index <= 0; index--)
		Employee[index] = Employee[index - 1];
	
}

void _company::printCompanyNames()
{
	std::cout << name << std::endl;
	for (int i = 0; i < total_employee; i++)
		Employee[i].printEmployeeNames();
}

here's the main driver:

        
#include <iostream>
#include <string>
#include "Ass_2.h"
#include "company.h"

using namespace std;

void CompanyListMenu(string[]);
int total_company = 0;

int main ()
{
	_company Company[20];
	_company UnEmployed;
	string CompanyNames[20];
	string EmpName, CompName;
	char choice;
	int CompID = 0;
	
	cout << "Please enter number of companies be listed (up to 20). ";
	cin >> total_company;
	while (total_company < 0 || total_company > 20){
		cout << "Please enter a number that is from 1 to 20.";
		cin >> total_company;
	}
	
	cout << "Enter the name of the company at each of the prompts." << endl;
	for (int i = 0; i < total_company; i++){
		cout << ">> ";
		cin >> CompName;
		CompanyNames[i] = CompName;
		Company[i].setCompanyName(CompName);
	}

	cout << "Please choose whether options: " << endl;
	cout << "(j)oin, (p)romote, (d)emote, (c)hange, (q)uit, (o)utput, (e)nd" << endl;
	cin >> choice;
	
	while (choice != 'e'){
		CompanyListMenu(CompanyNames);
		switch (choice){
		case 'j': {
			cout << "Enter company ID." << endl;
			cin >> CompID >> EmpName;
			// plan to have chart here for id and company reference (will be a function)

			Company[CompID].join_Company(EmpName);
			break;
		}
		case 'p': {
			cout << "Please enter employee up for promotion." << endl;
			cin >> EmpName;
			Company[CompID].promote_Employee(EmpName);
			break;
		}
		case 'd': {
			cout << "Please enter employee being demoted. " << endl;
			cin >> EmpName;
			Company[CompID].demote_Employee(EmpName);
			break;
		}
		case 'c': {
			int nCompID = 0;
			cout << "Employee to be transferred is: ";
			cin >> EmpName;
			Company[CompID].change_Employee(EmpName, CompName, Company[nCompID]);
			break;
		}
		case 'q': {
			cout << "Termination papers have just arrived. Name please." << endl;
			cin >> EmpName;
			Company[CompID].Employee_quits(EmpName, UnEmployed);
			cout << EmpName << "'s termination is complete!" << endl;
			break;
		}
		
		case 'o': {
			for (int i = CompID; i < total_company; i++)
					Company[CompID].printCompanyNames();
			break;
		}
		default: cout <<" Those options are not permitted. Please try again." << endl;
			break;
	}

	cout << "Please choose whether options: " << endl;
	cout << "(j)oin, (p)romote, (d)emote, (c)hange, (q)uit, (e)nd, (o)utput" << endl;
	cin >> choice;
}
			

	return 0;
}

void CompanyListMenu(string company_array[])
{
	for (int i = 0; i < total_company; i++)
		cout << i << '\t' << company_array[i] << endl;
	cout << endl <<endl;
}

again thanks! [edited by - Alpha_ProgDes on October 20, 2002 6:27:40 PM] [edited by - Alpha_ProgDes on October 20, 2002 11:55:55 PM]

Beginner in Game Development? Read here. And read here.

Alpha_ProgDes

6,963

Author

October 20, 2002 10:01 AM

i might post the complete code in this or the beginners forum if people think the code is well structured enough to be a model for other newbies/beginners to work with.

[edited by - Alpha_ProgDes on October 20, 2002 11:02:21 AM]

Beginner in Game Development? Read here. And read here.

Kylotan

10,510

October 20, 2002 11:25 AM

Ok...

The extern on the enum is almost certainly pointless.
The names in the enum are misleading: verbs imply actions, so they should probably be nouns or adjectives for states.
Underscores at the start of your identifiers is generally a Bad Idea as they can clash with library implementations.
Why is employeeSalary static? Why does rank change each time you call the calculateSalary function? This is misleading.
Why does calculateSalary return an int which you never use?
This should probably be split into 2 functions: one to alter an employees rank, and one to get their salary, which is const and changes nothing in the employee.
change_employee is just plain weird. Creating new companies should be done outside the existing company object, both for clarity purposes and because, right now, that new company is destroyed just a couple of lines later anyway.
And no, the code isn''t clear enough to not need comments. In fact, it would be far easier to judge the code if it had comments, because it would then be possible to spot the difference between what you actually intended and what you achieved. People who worry about comments going out of sync with the code forget that sometimes that''s the entire point - they force you to check that everything is still correct.

As for finding an employee, finding something inside a class should be done inside that class.

[ MSVC Fixes | STL | SDL | Game AI | Sockets | C++ Faq Lite | Boost | Asking Questions | Organising code files | My stuff ]

Qw3r7yU10p!

819

October 20, 2002 11:39 AM

Things I like: It''s clean and you use the standard headers (ie not iostream.h).

1) Don''t use underscore ''_'' at the beginning of anything. Ever. It''s reserved for compiler specific, vendor specific definitions no matter what you see in various places.

2) No need for <iostream> to be included for the current code - it''s not being used. However, even when you do need it then, in your header files, if it''s appropriate, use <iosfwd> which is all the definitions you need without all the actual code. You can then just include <istream>, or whatever the specific header you need is, in your .cpp file.

3) You could make the enumeration a member of Employee (and it doesn''t work as an extern anyway)

4) You have a global variable, status, which doesn''t really represent something globally true (and actually won''t work correctly as it''s declared either). It''s just the status of the current employee being processed. Use a local variable in the body of the function.

5) In your promote/demote functions they need to return the salary, otherwise the value being calculated isn''t being used and is just lost after Calculate Salary is called.

6) It''s good to initialise an object using an initialisation list. It''s quicker and tidier and less error prone.

7) Pass possibly large objects, such as std::string, by reference, which passes an address rather than the whole object. If you want to ensure (or tell the user) that you won''t change the original value then pass by constant reference. ie
employee::employee (const std::string& name). It''s almost always more efficient (unless you''re passing a basic type like an int or double which the machine can handle quickly anyway).

8) If the employee class is supposed to be used and passed around, like you would an integer value, then you need to include many more standard functions. Copy construction, assignment, equality, comparison, output: all these things are needed to make it a useful object. And I think in this example it is necessary.

These are comments on the current code and design as it stands. I would suggest far larger changes to make it a good example of application design, coding, function naming, object initialisation and class design etc.

Alpha_ProgDes

6,963

Author

October 20, 2002 12:02 PM

thank you very much for your comments

Kylotan:
employeeSalary is static because i want to keep track of the salaries of each employee. also it is directly tied in with the rank. when an employee is promoted, demoted or fired their rank changes and their salary is then altered.
as for status, yeah i realized it and fixed that. but it''s still extern, i''ll look into the suggestions you and petewood gave once it''s up and working.
changeEmployee just moves an employee from the current company and allows him to join a new company (that code is nowhere near completed i''m working on that now)
you said the new company is destroyed a couple of lines down. how?

petewood:
how is the value (which is employeeSalary) lost if i have it as static? did i miss something? i''ve never used an initialization list and therefore have no idea how it''s implemented. i never thought of passing string by reference (thank you, i''ll do that). i''m not too good with copy constructors (never really learned them that well). i''m plan and have to output, comparison and assignment in the program also.

(so i got two [good] points from petewood and a "oh boy, he has no idea what''s going on" from Kylotan)

thank you guys for your comments. i really appreciate it.

--VC6 is doing something funny. i added a new private variable to my company constructors then to my class declaration. when i compile VC6 says my new private variable are undeclared. anyone knows what''s going on with that?

Beginner in Game Development? Read here. And read here.

civguy

308

October 20, 2002 12:27 PM

quote:Original post by petewood
1) Don''t use underscore ''_'' at the beginning of anything. Ever. It''s reserved for compiler specific, vendor specific definitions no matter what you see in various places.

I''ve heard that this only applies to names that begin with ''_'' and are followed by either
a) another ''_''
b) an uppercase letter

I use _memberVariable to name my member variables (the same convention was used in Design Patterns -book)

Qw3r7yU10p!

819

October 20, 2002 12:44 PM

In change_Employee you create a local variable called anotherCompany, you give it a name, add an employee and then at the end of the function it goes out of scope and is destroyed. You must know this because when asked the question "how can you now refer to the new company?" you won''t have an answer. It''s gone.

What you need to do is pass in the ''other'' comany (by reference), remove the employee from the existing company and add them to the ''other'' company. If you want to do this in a way as an example to programmers of exception safe code there are lots of other things to do to make the code robust. You wouldn''t want the employee removed from one company and not added to the other company before an exception was thrown.

I only skimmed your code and was commenting more on style, so I missed the static value of salary. Better to put it as a member variable so other functions can access it rather than calculateSalary. getSalary would be a better name or even just salary to access the member. It can be recalculated by calculateSalary whenever something which affects it, changes.

Glad you appreciate the comments. It''s hard to show code publicly. Keep it up.

Alpha_ProgDes

6,963

Author

October 20, 2002 12:53 PM

yeah you''re right.
i had to change that anotherCompany to a reference parameter.
thanks.
yeah i thought people would come down from high heaven and completely scorch my code.
but thankfully there are caring programmers willing to help the less fortunate

so i''m a keep at it and post a new version (hopefully with comments if i don''t get too lazy) later.

actually in response to Kylotan. i just wanted to see how much readable code i could get in before having to put in comments.
i wrote the algorithm out. but i wanted to code in such a way you could read it like english or to a point that a beginner can easily follow the code.

again thanks!

Beginner in Game Development? Read here. And read here.

Arild Fines

968

October 20, 2002 01:11 PM

Why do you have a method called setEmployee to set the name?

The world holds two classes of men -- intelligent men without religion, and religious men without intelligence. - Abu''l-Ala-Al-Ma''arri (973-1057; Syrian poet)

--AnkhSVN - A Visual Studio .NET Addin for the Subversion version control system.[Project site] [IRC channel] [Blog]

Alpha_ProgDes

6,963

Author

October 20, 2002 01:21 PM

would you rather have me name the function differently or have a completely different function for that task?

options: getEmployeeName, setEmployeeName, initializeName :D

Beginner in Game Development? Read here. And read here.

My Code: Organized or Hell in Ascii??

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

My Code: Organized or Hell in Ascii??

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines