• Announcements

Archived

This topic is now archived and is closed to further replies.

My Code: Organized or Hell in Ascii??

Recommended Posts

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;

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'){
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;
}

{
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]

Share on other sites
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]

Share on other sites
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 ]

Share on other sites
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.

Share on other sites

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)

--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?

Share on other sites
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)

Share on other sites
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.

Share on other sites
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!

Share on other sites
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)

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

options: getEmployeeName, setEmployeeName, initializeName :D

Share on other sites
How about setName? setEmployeeName is redundant since it''s already on an Employee object.

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)

OK!

Share on other sites
i love the design patterns book and that was explictly what i was thinking of when i said ''no matter what you see in various places''.

Share on other sites
Not asking for homework help yet it looks suspiciously like Ass_2.h means Assignment 2 Header

Share on other sites
a bigger design point -

it is actually quite unlikely that you will need to change an employee''s name. it''s also possible that you will have records associated with an employee''s name. maybe the name should become unimportant and the employee should have a unique id (which wouldn''t change). then records, such as the employee''s name would be kept in a seperate area and records would be referenced by the object or the id etc.

keep the interface minimal and realistic. if you''ll never have to set the employee name don''t put in an explicit function. if you do have to set it, make sure that the side-effects are understood.

Share on other sites
Grr

I''m on my weekend. Tell me it''s not true Alpha_ProgDes.

Share on other sites
redstar1:
it is a homework assignment !
but notice i said (paraphrasing) "how can i make it better? am i on the right track. suggestions and comments welcome."

not! "hey i got some homework and someone tell me how to code it?"

petewood:
true.
once the user inputs the name then that will not change.
(i''ve always wondered if there is a keyword in C++ that allows you to cin a value then lock that variable with a const right afterwards, but that''s another discussion) the id thing sounds good. because it seems that search the employee list with an inputted employee name is not gonna work so well. integer may work better.

Share on other sites
petewood:
(and everyone else

if i have upset you, i am sorry.
i thought i was clear, it was not my intention to mislead anyone.
this is an assignment that I am doing.
i wanted the input of others in the forum because i intend to work on it and refine after i hand it in.
obviously from the posts i have read my "way" is not exactly programmer efficient. but the only way to learn and hone my skills (what very little i have of them ), is to ask.
so if other are bothered this fact, then please do not bother yourself with this as i know there are more interesting and challenging coding problem being posted almost every minute.
for those of you who will help me, again i appreciate your help & guidance and comments & suggestions.

[edited by - Alpha_ProgDes on October 20, 2002 4:12:00 PM]

Share on other sites
you''ve removed your original post which said you were thinking of making it a tutorial for beginners. this made me think it was original code that you were preparing.

saying ''i''m not asking for help on my homework'' is easily interpreted as saying ''this isn''t my homework''.

maybe i should start supplying you with untrue answers to your untrue questions...

but i won''t become cynical. just be aware that all this community runs on good will: your and my actions contribute for good or bad.

if you''d said it was your homework i''d have answered in a different way, probably more beneficial to you and your learning. so it''s your loss. do keep coming back though... everyone needs a second chance (especially me)

peace

Share on other sites
again.
i thought i was clear.
next time i''ll be more specific in my wording.
anyway thanks again for the help.
though i am bothered by "maybe i should start supplying you with untrue answers to your untrue questions..."
i didn''t and don''t think anything i said was untrue.

but again it was a misunderstanding which was my fault.
so everyone enjoy your weekend and thanks for the help!!

Share on other sites
I don''t see how it matters whether it was homework or not - he didn''t ask people to write it for him or anything. As far as I am concerned, his post was legitimate.

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)

Share on other sites
it seems like my variables are not getting into scope.
the debugger tells me this but i have no idea why.
if anyone can give me some insight it would be greatly appreciated.

note: this is code is work in progress.

Share on other sites
2 words: const correctness

I know it doesn''t matter a lot in a small project but it''s a good habit to get into. methods that return information about the state of an object (i.e. pretty much anything with "get" or "is" at the start of it) should be const.

Share on other sites
quote:
Original post by ChaosEngine
I know it doesn''t matter a lot in a small project but it''s a good habit to get into. methods that return information about the state of an object (i.e. pretty much anything with "get" or "is" at the start of it) should be const.
Only applies to stuff that is passed by reference or by pointer, since those could be changed. If the return value is a copy, it doesn''t have to be const.

Share on other sites
Arild Fines:

in case you didn''t read the original post before it was edited it said something along the lines of:

this is code i''ve come up with. i''ve made it obvious and good so i can post it on the beginners forums as an example of good code. has anyone got any suggestions how to improve it?

i was horrified that it would be posted as it stood.

anyway, i''m not going to keep grinding the axe, but this discussion is skewed because you can''t see the original post.

• Forum Statistics

• Total Topics
627706
• Total Posts
2978720

• 21
• 14
• 12
• 22
• 35