Help + proofread progam

[sfurlow72]

Alright, this isn't actually homework. I'm going back to school so I figured I would brush up on some programming before I go. I'll be taking the second course in a three course C++ programming sequence (starts with classes, recursion, etc.), so nothing too advanced yet. I wrote this program to review classes and input/output streams. It should read in three numerical values from a file, do some basic manipulations, then output everything. I was wondering if anyone would like to proofread it and give me some suggestions as to how it could be better? Thanks. Also, a minor problem I'm having is that the output isn't showing two digits of precision.

header file:

#include <iostream>
#include <string>

using namespace std;

#ifndef SALARY_H
#define SALARY_H

const double MINRAISE = .05;
const int MINSALARY = 15000;
const double MIDRAISE = .07;
const double MAXRAISE = .1;
const int MAXSALARY = 50000;
const int WEEKS = 52;
const int MONTHS = 12;
const int MIN = 0;
const int DEP = 4;
const int MAX_ID= 999;

class Salary
{
	public: Salary();
			Salary(int dep, int ID, double sal);
			const int getDepNum();
			const int getWorkerID();
			const double getYearlySalary();
			const string getDepartment();
			void setDepNum(int x);
			void setWorkerID(int x);
			void setYearlySalary(double x);
			void setDepartment(int x);
			const double newSalary();
			const double monthlySalary();
			const double weeklySalary();

			

	private: int depNum, workerID;
			 double yearlySalary;
			 string department;
	friend istream& operator >> (istream &in, Salary &x);
	friend ostream& operator << (ostream &out,  Salary &x);
			 

			
};

#endif

Implementation File

#include <iostream>
#include <string>
#include "salary_header.h"
using namespace std;

Salary::Salary(int dep, int ID, double sal)
{
	depNum = dep;
	workerID = ID;
	yearlySalary = sal;
}

const int Salary::getDepNum() 
{
	return depNum;
}
const int Salary::getWorkerID()  
{
	return workerID;
}
const double Salary::getYearlySalary()  
{
	return yearlySalary;
}
const string Salary::getDepartment()
{
	return department;
}
void Salary::setDepNum(int x)
{
	if(	(x > MIN) && (x <= DEP))
		depNum = x;
	else
		depNum = 1;
}
void Salary::setWorkerID(int x)
{
	if(	(x > MIN) && (x <= MAX_ID))
		workerID = x;
	else 
		workerID = MIN;
}
void Salary::setYearlySalary(double x)
{
	if(	(x > MIN))
		yearlySalary = x;
	else
		yearlySalary = MIN;
}
void Salary::setDepartment(int x)
{
		if(x==1)
			department = "Computer Science";
			
		else if(x==2) 
			department = "Business Administration";
			
		else if(x==3)
			department = "Accounting";
			
		else if(x==4)
			department = "Electrical Engineering";
			
		else 
			department = "Invalid Department";
			
}
const double Salary::newSalary() 
{
	if(yearlySalary < MINSALARY)
		  return ((yearlySalary*MINRAISE) +  yearlySalary);
	else if(	(yearlySalary >= MINSALARY)&&(yearlySalary < MAXSALARY))
		 return ((yearlySalary*MIDRAISE) + yearlySalary);
		
	else
		return ((yearlySalary*MAXRAISE) + yearlySalary);
}

const double Salary::monthlySalary() 
{
	return (newSalary()/MONTHS);
}
const double Salary::weeklySalary() 
{
	return (newSalary()/WEEKS);
}
ostream& operator << (ostream &out,  Salary &x)
{
	cout.setf(ios::fixed);
	cout.precision(2);
	cout.showpoint;

	x.setDepartment(x.getDepNum());

	out << "Employee ID: " << x.getWorkerID() << endl;
	out << "Department: " << x.getDepartment() << endl;
	out << "Old Salary: " << x.getYearlySalary() << endl;
	out << "New Salary: " << x.newSalary() << endl;
	out << "New Monthly Salary: " << x.monthlySalary() << endl;
	out << "New Weekly Salary: " << x.weeklySalary() << endl << endl << endl;
	
	return out;
}

istream& operator >> (istream &in, Salary &x)
{
	
	in >> x.depNum >> x.workerID >> x.yearlySalary;

	return in;
}

Driver

#include <iostream>
#include <fstream>
#include <string>
#include "salary_header.h"

using namespace std;	

void readWrite(Salary& salary);


int main()
{
	Salary salary(0,0,0);
	int userDep(0), userID(0);
	double userSalary(0);
	readWrite(salary);
	
	system("PAUSE");
	return 0;
}

void readWrite(Salary& salary)		
{
	ifstream in;
	ofstream out;
	in.open("data.txt");
	out.open("OutputData.txt");

	if(in.fail())
	{
		cout << "File opening failed";
		exit(1);
	}
	
	while (!(in.eof()))
	{
		in >> salary;  
		out << salary;
	}
	in.close();
	out.close();
}

[ravenous]

I would try and have a play with the list of global constants that you have at the top of salary.h. It looks like some of them could be incorporated into the Salary class itself. Specifically, MINRAISE , MINSALARY , MIDRAISE , MAXRAISE and MAXSALARY . You could make them static const members of Salary (either as a function or variable):

class Salary{
public:
   static const double min_raise;
   
/* or */

   static double maxRaise() { return 0.1; }

/* rest of class details here */
};

const double Salary::min_raise = 0.05;

Then your code is more readable, since it is explicitly stated that the "minimum raise" is applied to a salary, and not, say, the cost of a consumable or something.

You have declared several of the member functions in Salary like this:

const int getDepNum();

There's not really much point in doing this, since the function is returning by value. If you want to express that the function doesn't change anything in the class, then you need a const after the function too. So, I think you probably mean:

int getDepNum() const;

You should pass the Salary object to the extraction function by const reference. You have:

friend ostream& operator << (ostream &out, Salary &x);

But you probably want:

friend ostream& operator << (ostream &out, const Salary &x);

You also don't need to have operator >> as a friend, just overload it outside the class and use the Salary 's accessor functions to set the details:

class Salary{
   /* stuff */
};

istream& operator >> (ostream &out, Salary &x);

istream& operator >> (istream &in, Salary &x)
{
   double dep, ID, sal;
   in >> dep >> ID >> sal;

   x.setDepNum( dep );
   x.setWorkerID( ID );
   x.setYearlySalary( sal );
   
   return in;
}

It's also a bit of a tautology to have a functions called things like getYearlySalary in a Salary class. Really the thing that you're getting here is the value of the Salary object. A name like getYealyValue might be a clearer name.

You can use an initializer list to set the values of the members of Salary :

Salary::Salary(int dep, int ID, double sal) :
   depNum( dep ), workerID( ID ), yearlySalary( sal )
{
}

Make the department variable an enum . If you're going to have to convert the enum to text, I like to make a small class to do this:

class Department
{
   enum EType {
      unknown =  -1,
      accounting = 0,
      business_admin = 1,
      computer_science = 2,
      /* etc */
      number_of_departments
   };

   static const char* ToText( Department::EType t )
   {
      switch( t )
      {
          case accounting : return "Accounting";
          case business_admin : return "Business Administration";
          case computer_science : return "Computer Science";
          default : return "Unknown";
      }
   }

   static EType FromText( const std::string& s )
   {
      for( int i = -1; i < static_cast< int >( number_of_departments ); ++i )
         if ( s == Department::ToText( static_cast< EType >( i ) )
            return static_cast< EType >( i );
      
      return EType::unknown;
   }
};

This way, you can declare variables like Department::EType eDep; and convert to and from text using std::string strDep = Department::ToText( eDep ); Hope that's given you some ideas :)

[sfurlow72]

Yeah, thanks a lot. That gave me a lot to think about. I haven't had a chance to work on that stuff yet, but I had a question about the use of const. You said:

You have declared several of the member functions in Salary like this:

const int getDepNum();

There's not really much point in doing this, since the function is returning by value. If you want to express that the function doesn't change anything in the class, then you need a const after the function too. So, I think you probably mean:

int getDepNum() const;

This is what I meant to do in the first place (for some reason it didn't compile so I changed it, thinking that I did it wrong in the first place). Could you explain the difference between declaring a const function and placing const after a function?

[ravenous]

Could you explain the difference between declaring a const function and placing const after a function?

Putting the const after the function is declaring the function constant. When you have the const at the beginning, you are saying that the return-type of the function is const , not the function itself. The first is a guarantee that calling the function will not change the state of the object at all (except for member variables that are declared mutable ). Whereas, the second does not make any guarantees about what it will do to the object itself, but does mean that you can't alter the local value that is returned.

A common pattern that sees use of both consts at the same time is returning something by reference:

const MyObject& getObject() const;