Homework help: is this a good design?
Hi guys,
I'd like some advice on my C++ homework, which is to create an Employee class with three data members (first name, last name, and salary), accessor methods for each, and a function to display all three data members. My instructor isn't very picky about how we implement his requirements, just that it gives the results he's looking for.
Here's my design:
Employee.hpp
Code:
//inclusion guards
#ifndef EMPLOYEE_HPP
#define EMPLOYEE_HPP
//including string header file
#include <string>
using std::string;
//Employee class declaration
class Employee
{
public:
//ctor
Employee(string fn,string ln,double s);
//cctor
Employee(const Employee& rhs);
//move ctor
Employee(Employee&& rhs);
//copy assignment operator
virtual Employee& operator=(const Employee& rhs);
//move assignment operator
virtual Employee& operator=(Employee&& rhs);
//~dtor
virtual ~Employee();
//getters and setters
virtual string& GetFirstName() const;
virtual void SetFirstName(const string& fn);
virtual string& GetLastName() const;
virtual void SetLastName(const string& ln);
virtual double& GetSalary();
virtual void SetSalary(const double& s);
//fn to display Employee data
virtual void DisplayEmployee() const;
private:
struct EmployeeData;
EmployeeData* data;
};
#endif
Employee.cpp
Code:
//headers for string and I/O
#include <iostream>
#include <string>
using std::cout;
using std::endl;
using std::string;
//include Employee class declaration
#include "Employee.hpp"
//struct to hold Employee data
struct EmployeeData
{
EmployeeData(string fn,string ln,double s);
EmployeeData(const EmployeeData& rhs)=default;
EmployeeData& operator=(const EmployeeData& rhs)=default;
string firstname;
string lastname;
double salary;
};
//EmployeeData ctor definition
EmployeeData::EmployeeData(string fn,string ln,double s)
{
if (s < 0)
salary=0.00;
else
{
firstname=fn;
lastname=ln;
salary=s;
}
}
//Employee ctor definition
Employee::Employee(string fn = "unknown",string ln = "unknown",double s = 0.0):data(new EmployeeData(fn,ln,s))
{
}
//Employee cctor def
Employee::Employee(const Employee& rhs):data(new EmployeeData(rhs.*data))
{
}
//Employee move ctor def
Employee::Employee(Employee&& rhs):data(new EmployeeData(rhs.*data))
{
//null rhs's data ptr to prevent needless memory deallocation when rhs goes out of scope
rhs.data=0;
}
//Employee copy assignment operator def
virtual Employee& Employee::operator=(const Employee& rhs)
{
//check for self-assignment
if(this==&rhs)
return this;
else
this.*data=rhs.*data;
return this;
}
//Employee move assignment operator def
virtual Employee& Employee::operator=(Employee&& rhs)
{
//check for self-assignment
if(this==&rhs)
return this;
else
this.*data=rhs.*data;
//null rhs's data ptr to prevent needless memory deallocation when rhs goes out of scope
rhs.data=0;
return this;
}
//Employee dtor def;
virtual Employee::~Employee()
{
if(data) //only deallocate memory if data is a null ptr
delete data;
}
virtual string& Employee::GetFirstName() const
{
return data->firstname;
}
virtual void Employee::SetFirstName(const string& fn)
{
data->firstname=fn;
}
virtual string& Employee::GetLastName() const
{
return data->lastname;
}
virtual void Employee::SetLastName(const string& ln)
{
data->lastname=ln;
}
virtual double& Employee::GetSalary() const
{
return data->salary;
}
virtual void Employee::SetSalary(const double& s)
{
if(s >= 0)
data->salary=s;
else
data->salary=0.0;
}
virtual void Employee::DisplayEmployee() const
{
cout << "First Name: " << /t << data->firstname << endl;
cout << "Last Name: " << /t << data->lastname << endl;
cout << "Salary: " << /t << data->salary << endl;
}
Driver.cpp
Code:
/include iostream and string headers
#include <iostream>
#include <string>
using std::cout;
using std::cin;
using std::endl;
using std::string;
//include Employee class def
#include "Employee.hpp"
//begin driver
int main()
{
//variables for employee's first and last names and salary
string FirstName;
string LastName;
double salary;
//read user input
cout << "Enter an employee's first and last name, pressing /"enter/" after each name." << endl;
cin >> FirstName >> LastName;
cout << "Enter the employee's salary." << endl;
cin >> salary;
//create an Employee using the input
Employee officedrone(FirstName,LastName,salary);
//display the Employee's fields
officedrone.DisplayEmployee();
//ask user to modify the Employee's salary
cout << officedrone.GetFirstName() << " " << officedrone.GetLastName() << " has received a promotion!"
<< "Please enter his new salary: " << endl;
cin >> salary;
officedrone.SetSalary(salary);
cout << officedrone.GetFirstName() << " " << officedrone.GetLastName() << " has decided to use his newfound income to get a sex change! What name should she now be known as?" << endl;
cin >> FirstName >> LastName;
officedrone.SetFirstName(FirstName);
officedrone.SetLastName(LastName);
//display the modified Employee's fields
officedrone.DisplayEmployee();
//All done! Return to OS!
return 0;
}
The assignment is to demonstrate our understanding of basic OOP principles.
Also, wish me luck on my C++ test tonight!
Re: Homework help: is this a good design?
The only slight niggle that jumps out at me right away is that you've made all your functions virtual. In the case of getting and setting the names and probably the salary, that doesn't seem necessary.
Re: Homework help: is this a good design?
He told us we'll be inheriting from Employee in later assignments.
Re: Homework help: is this a good design?
Quote:
Originally Posted by
TSLexi
He told us we'll be inheriting from Employee in later assignments.
That doesn't mean all your functions need to be virtual. Only make them virtual if you think their behavior will change in derived classes. GetFirstName would be the same regardless of the employee class I would think, therefore you'll want the behavior of the parent class.
Re: Homework help: is this a good design?
Also, should my functions pass arguments by value, by const lvalue ref, or by rvalue ref?
Re: Homework help: is this a good design?
that question depends entirely on what your expectancies are.
there are reasons for all 3, and reasons why any of the 3 could be inappropriate in a given context.
typically speaking though:
If you can pass by const reference and this is the behaviour you want, then do so
if you need to change the object, use a non-const reference.
only use pass by value if you explicitely want this to be what's happening (= operating on a copy)
only use pass by rvalue ref if you intend to "move ownership" of members.
you forgot about pass by pointer, which is good. pointers are "bad". only use them when none of the above is adequate.
Re: Homework help: is this a good design?
Code:
//Employee dtor def;
virtual Employee::~Employee()
{
// if(data) no need for this
delete data;
}
There is no need to check if data is not null to delete it. It is perfectly safe to call delete on a null pointer.
Regards,
Paul McKenzie
Re: Homework help: is this a good design?
Another thing, in most cases functions don't want to return references. That applies here.
Re: Homework help: is this a good design?
Quote:
Originally Posted by
Paul McKenzie
Code:
//Employee dtor def;
virtual Employee::~Employee()
{
// if(data) no need for this
delete data;
}
There is no need to check if data is not null to delete it. It is perfectly safe to call
delete on a null pointer.
Regards,
Paul McKenzie
Similarly.. be sure you always initialize the data to null in the constructor.
Re: Homework help: is this a good design?
Quote:
Originally Posted by
TSLexi
Hi guys,
I'd like some advice on my C++ homework, which is to create an Employee class with three data members (first name, last name, and salary),
Personally I wouldn't return references in your class. Just return the data.
The reason is that first, the data in your class represent "simple" things -- a salary, a name, etc. The data isn't a complex object, where it doesn't make sense for that object to survive beyond its containing object. So for things such as strings, integers, double, etc. return the data.
Another reason is that if that Employee object goes out of scope and your code is holding onto one of the values that was returned by reference, you are now referencing an invalid string. You now have a bug in your program, and for what reason? Again, return the data, not a reference, and then you will be ok.
Regards,
Paul McKenzie
Re: Homework help: is this a good design?
OK, here's the final version (I modified the move ctor so it just switches the pointers around):
Employee.hpp
Code:
//inclusion guards
#ifndef EMPLOYEE_HPP
#define EMPLOYEE_HPP
//including string header file
#include <string>
using std::string;
class EmployeeData;
//Employee class declaration
class Employee
{
public:
//ctor
Employee(string fn,string ln,double s);
//cctor
Employee(const Employee& rhs);
//move ctor
Employee(Employee&& rhs);
//copy assignment operator
virtual Employee operator=(const Employee& rhs);
//move assignment operator
virtual Employee operator=(Employee&& rhs);
//~dtor
virtual ~Employee();
//getters and setters
virtual string GetFirstName()const;
virtual void SetFirstName(const string& fn);
virtual string GetLastName()const;
virtual void SetLastName(const string& ln);
virtual double GetSalary() const;
virtual void SetSalary(const double& s);
//fn to display Employee data
virtual void DisplayEmployee() const;
private:
EmployeeData* data;
};
#endif
Employee.cpp
Code:
//headers for string and I/O
#include <iostream>
#include <iomanip>
#include <string>
using std::cout;
using std::endl;
using std::setprecision;
using std::fixed;
using std::string;
//include Employee class declaration
#include "Employee.hpp"
//struct to hold Employee data
class EmployeeData
{
public:
EmployeeData(string fn,string ln,double s);
EmployeeData(const EmployeeData& rhs)=default;
EmployeeData& operator=(const EmployeeData& rhs)=default;
string firstname;
string lastname;
double salary;
};
//EmployeeData ctor definition
EmployeeData::EmployeeData(string fn,string ln,double s)
{
firstname=fn;
lastname=ln;
if(salary < 0)
salary=0.0;
else
salary=s;
}
//Employee ctor definition
Employee::Employee(string fn = "unknown",string ln = "unknown",double s = 0.0):data(new EmployeeData(fn,ln,s))
{
}
//Employee cctor def
Employee::Employee(const Employee& rhs):data(new EmployeeData(*(rhs.data)))
{
}
//Employee move ctor def
Employee::Employee(Employee&& rhs):data(0)
{
data=rhs.data;
//null rhs's data ptr to prevent needless memory deallocation when rhs goes out of scope
rhs.data=0;
}
//Employee copy assignment operator def
Employee Employee::operator=(const Employee& rhs)
{
//check for self-assignment
if(this==&rhs)
return *this;
else
*(this->data)=*(rhs.data);
return *this;
}
//Employee move assignment operator def
Employee Employee::operator=(Employee&& rhs)
{
//check for self-assignment
if(this==&rhs)
return *this;
delete data;
data=rhs.data;
//null rhs's data ptr to prevent needless memory deallocation when rhs goes out of scope
rhs.data=0;
return *this;
}
//Employee dtor def;
Employee::~Employee()
{
if(data) //only deallocate memory if data is a valid ptr
delete data;
}
string Employee::GetFirstName()const
{
return data->firstname;
}
void Employee::SetFirstName(const string& fn)
{
data->firstname=fn;
}
string Employee::GetLastName()const
{
return data->lastname;
}
void Employee::SetLastName(const string& ln)
{
data->lastname=ln;
}
double Employee::GetSalary()const
{
return data->salary;
}
void Employee::SetSalary(const double& s)
{
if(s >= 0)
data->salary=s;
else
data->salary=0.0;
}
void Employee::DisplayEmployee()const
{
cout << "First Name: " << data->firstname << endl;
cout << "Last Name: " << data->lastname << endl;
cout << "Salary: " << fixed << setprecision(2) << data->salary << endl;
}
Driver.cpp
Code:
//include iostream and string headers
#include <iostream>
#include <string>
using std::cout;
using std::cin;
using std::endl;
using std::string;
//include Employee class def
#include "Employee.hpp"
//begin driver
int main()
{
//variables for employee's first and last names and salary
string FirstName;
string LastName;
double salary;
//read user input
cout << "Enter an employee's first and last name, pressing <" << "enter" << "> after each name." << endl;
cin >> FirstName >> LastName;
cout << "Enter the employee's salary." << endl;
cin >> salary;
//create an Employee using the input
Employee officedrone(FirstName,LastName,salary);
//display the Employee's fields
officedrone.DisplayEmployee();
//ask user to modify the Employee's salary
cout << officedrone.GetFirstName() << " " << officedrone.GetLastName() << " has received a promotion!"
<< "Please enter his new salary: " << endl;
cin >> salary;
officedrone.SetSalary(salary);
cout << officedrone.GetFirstName() << " " << officedrone.GetLastName() << " has decided to use his newfound income to get a sex change! What name should she now be known as?" << endl;
cin >> FirstName >> LastName;
officedrone.SetFirstName(FirstName);
officedrone.SetLastName(LastName);
//display the modified Employee's fields
officedrone.DisplayEmployee();
//All done! Return to OS!
return 0;
}
Re: Homework help: is this a good design?
Quote:
Originally Posted by
TSLexi
OK, here's the final version
A final touch would be to remove everything virtual.
As newbie (and well beyond that) you only have to concern yourself with how to implement two types of classes cleanly. These are value classes and OO polymorphic classes.
Value classes are modelled after the primitives like int and double and include most of the standard library classes such as std::string and std:vector. They all have copy semantics and they're not intended to be inherited from.
As it's implemented, Employee is a typical value class. Using virtual methods doesn't make it an OO polymorphic class. It's just bogus marketing.
So make Employee a proper value class for this assignment. Then in later assigments (when Employee is to be inherited from) change the design to a proper OO polymorphic class. It won't have copy semantics and it won't carry state (data).
Finally note that your implementation of copy sematics for the Employee class becomes much simpler if Employee would hold the EmployeeData object by value rather than by pointer. It's because all members of EmployeeData ara themselves value classes which means the default copy machinery of Employee (the default copy constructor and the default copy assignment operator) will then automatically supply the copy semantics. It's like for free. But it's much less of an exercise of course.
Re: Homework help: is this a good design?
Code:
//EmployeeData ctor definition
EmployeeData::EmployeeData(string fn,string ln,double s)
{
firstname=fn;
lastname=ln;
if(salary < 0)
salary=0.0;
else
salary=s;
}
I think you have a logic error here as you testing salary rather than s? I think you mean
Code:
//EmployeeData ctor definition
EmployeeData::EmployeeData(string fn,string ln,double s) : firstname(fn), lastname(ln), salary(s < 0 ? 0 : s) {}
operator=() usually returns a reference so that statements like below can be written
Code:
emp1 = emp2 = emp3;
Code:
void Employee::SetSalary(const double& s)
{
if(s >= 0)
data->salary=s;
else
data->salary=0.0;
}
can be slightly simplied to
Code:
void Employee::SetSalary(const double& s)
{
data->salary = (s >= 0.0) ? s : 0.0;
}
PS The code would be easier to read if it was indented properly.
PPS Hope your c++ test went OK.
Re: Homework help: is this a good design?
Quote:
Originally Posted by
Paul McKenzie
Personally I wouldn't return references in your class. Just return the data.
Yet another aspect is design.
The only reason for the use of getter/setter methods is to increase the level of encapsulation by hiding implementation. When the getter is returning a reference to internal data that motivation is gone. The getter/setter serves no purpose any longer and could as well be removed.
Re: Homework help: is this a good design?
Quote:
Originally Posted by
TSLexi
Also, should my functions pass arguments by value, by const lvalue ref, or by rvalue ref?
The choise between by value and by const reference is a question of efficiency alone. Pass by value involves a copy of the parameter to be produced and the more this costs the more attractive pass by const reference becomes. The "cost" is not only the size of the data but also other calculations that may be performed (such as for example incrementing the counter in case of a smart pointer).
A common rule of thumb is that primitives are passed by value and everything more expensive is by const reference. Accordingly a double should be passed by value and a string by const reference.