Programming Best Practices

[dudeman007]

Hi Everyone,
I am trying to improve on how I write a program, I feel I may be picking up some bad habits from class and want to make sure I correct them. Here is my latest assignment, and suggestions on how to make it better? It builds and runs fine, I am looking at it from the point as if it would go to another programmer to utilize in a bigger program or even modify. Thanks for your feedback!!

Header File
// Preprocessor Directives
#ifndef EMPLOYEE_H
#define EMPLOYEE_H
#include <iostream>
#include<iomanip>

using namespace std;
//------------------------------------------------------------------------------
// Define Employee Class
class Employee
{
// Declare Public Data Members
public:
// Constructors and Destructor
Employee();
Employee(string, string, char, int, double);
~Employee();

//Methods to Access Attributes
double calculatePay();
void displayEmployee();

// Getters and Setters
string getFirstName();
void setFirstName(string);
string getLastName();
void setLastName(string);
char getGender();
void setGender(char);
int getDependents();
void setDependents(int);
double getAnnualSalary();
void setAnnualSalary(double);

// Declare Private Data Members
private:
string firstName;
string lastName;
char gender;
int dependents;
double annualSalary;
string input;
};

#endif;
//------------------------------------------------------------------------------

Source File
// Preprocessor Directives
#include "Employee.h"
#include <iostream>
#include <string>
#include <iomanip>

using namespace std;
//------------------------------------------------------------------------------
// Declare Prototypes
string getFirstName(string);
string getLastName(string);
char getGender(char);
int getDependents(int);
double getAnnualSalary(double);
void displayFirstName(string);
void displayLastName(string);
void displayGender(char);
void displayDependents(int);
void displayAnnualSalary(double);
void DislayApplicationInformation();
void DisplayDivider(string);
void DisplayDivider2(string);
string GetInput (string);
void TerminateApplication();
//------------------------------------------------------------------------------
// Main Function
int main()
{
// Declare Function Variables
string tempString;
char gender;
int dependents = 0;
double annualSalary;


DislayApplicationInformation();

DisplayDivider("Employee 1");

Employee employee1;

string firstName = GetInput("First Name");
employee1.setFirstName(firstName);

string lastName = GetInput("Last Name");
employee1.setLastName(lastName);

tempString = GetInput("Gender");
gender = tempString.c_str()[0];
employee1.setGender(gender);

tempString = GetInput("Number of Dependents");
dependents = atoi(tempString.c_str());
employee1.setDependents(dependents);

tempString = GetInput("Annual Salary");
annualSalary = atof(tempString.c_str());
employee1.setAnnualSalary(annualSalary);

DisplayDivider2("Employee Information");
employee1.displayEmployee();
cout << endl;


Employee employee2("Mary", "Noia", 'F', 5, 24000.00);
DisplayDivider("Employee 2");
DisplayDivider2("Employee Information");
employee2.displayEmployee();

TerminateApplication();
employee1.~Employee();
employee2.~Employee();
system("pause");

return 0;
}
//------------------------------------------------------------------------------
// Default Employee Constructor
Employee::Employee(void)
{
firstName = "not given";
lastName = "not given";
gender = 'U';
dependents = 0;
annualSalary = 20000;
};
//------------------------------------------------------------------------------
// Multi-Arg Employee Constructor
Employee::Employee(string inFirstName, string inLastName, char inGender, int inDependents, double inAnnualSalary)
{
firstName = inFirstName;
lastName = inLastName;
gender = inGender;
dependents = inDependents;
annualSalary = inAnnualSalary;
};
//------------------------------------------------------------------------------
// Employee Deconstructor
Employee::~Employee()
{
};
//------------------------------------------------------------------------------
// Define CalculatePay Function
double Employee::calculatePay()
{
return annualSalary/52;
}

// DisplayEmployee Function
void Employee::displayEmployee()
{

cout << "Employee Name:\t\t" << firstName << " " <<lastName <<endl;
cout << "Employee Gender:\t" << gender << endl;
cout << "Employee Dependents:\t" << dependents << endl;
cout << "Employee Annual Salary:\t$" <<
setprecision(2) << showpoint << fixed << annualSalary << "\n";
cout << "Employee Weekly Pay:\t$" << annualSalary/52 << endl;
};
//------------------------------------------------------------------------------
// Define GetFirstName and SetFirstName
string Employee::getFirstName()
{
return firstName;
}

void Employee::setFirstName(string newFirstName)
{
firstName = newFirstName;
}

string Employee::getLastName()
{
return lastName;
}

void Employee::setLastName(string newLastName)
{
lastName = newLastName;
}
//------------------------------------------------------------------------------
// Define GetGender and SetGender
char Employee::getGender()
{
return gender;
}

void Employee::setGender(char newGender)
{
gender = newGender;
}
//------------------------------------------------------------------------------
// Define GetDependents and SetDependents
int Employee::getDependents()
{
return dependents;
}

void Employee::setDependents(int newDependents)
{
dependents = newDependents;
}
//------------------------------------------------------------------------------
// Define GetAnnualSalary and SetAnnualSalary
double Employee::getAnnualSalary()
{
return annualSalary;
}

void Employee::setAnnualSalary(double newAnnualSalary)
{
annualSalary = newAnnualSalary;
}
//------------------------------------------------------------------------------
// DisplayApplicationInformation Procedure
void DislayApplicationInformation()
{
// Display Program Header
cout << "Welcome to your first Object Oriented Program" << endl;
cout << "Employee Class CIS247C, Week 2 Lab" << endl;
cout << "Name: Jim Stevens" << endl;
};
//------------------------------------------------------------------------------
// DisplayDivider Prodedure
void DisplayDivider(string outputTitle)
{
// Display Divider with Output Title
cout << "\n***************************" << outputTitle <<"***************************" << endl;
};
//------------------------------------------------------------------------------// DisplayDivider2 Prodedure
void DisplayDivider2(string outputTitle)
{
// Display Divider with Output Title
cout << "\n" << outputTitle << endl;
cout << "\n________________________________________________________________" << endl;
};
//------------------------------------------------------------------------------
// GetInput Function
string GetInput (string inputType)
{
// Declare Function Varaible
string input;
// Prompt User for Input
cout<<"Please enter your "<< inputType <<": ";
getline(cin, input);
return input;
};
//------------------------------------------------------------------------------// TerminateApplication Procedure
void TerminateApplication()
{
// Display Termination Message
cout << "\nThank you for using the Employee Class program\n";
};
//------------------------------------------------------------------------------

[jlb]

My first suggestion is to use code tags when posting code in the forum to make your code readable.

Second the following is a very bad practice:

Header File

using namespace std;

You should never use a using statement inside a header file. Use the scope resolution operator:: to properly scope the namespace inside headers for example std::string test;

Next you include several unnecessary headers in your header file, and fail to include a required header file. You don't need either <iostream> or <iomanip> in employee.h but you do need the <string> include file.

Next I would recommend you think about overloading the insertion operator<< to print your class. And start using constructor initialization lists instead of initializing the variables in the body of the constructor. http://www.learncpp.com/cpp-tutorial/101-constructor-initialization-lists/

[LowestOne]

My first suggestion is to use code tags when posting code in the forum to make your code readable

Also indent.

Generally, for school, using namespace std; is fine. If you're making something for yourself:

#include <iostream>

int main(void)
{
  int x;
  std::cin >> x;
  if (x == 5)
    std::cout << "You did it" << std::endl;
  return 0;
}

You don't need semi-colons after the function definition, only the declaration:

int myFunc(void);

int myFunc(void)
{
  return 0;
}

Also, don't put the includes in the .cpp, just the header.

[jlb]

Also, don't put the includes in the .cpp, just the header.

Actually I recommend that you include the proper include files in the location where these files are required. For example if you use the std::string class in a header either include the <string> in the header or if possible forward declare this class in your header. If you use the std::string class in a .cpp file include the <string> header in that file also. Never rely on some other file including an include file for you.

Generally, for school, using namespace std; is fine. If you're making something for yourself:

The OP was asking about best practices, it is never a good practice to use the using statement in a header file, however for small projects it is acceptable to use this statement in a .cpp file. However in my opinion you should get used to using the scope resolution operator:: everywhere.

[Framework]

jlb wrote:

"it is never a good practice to use the using statement in a header file"

That's an ambiguous statement since "using" has different meanings in different contexts. In some contexts, "using" is not bad practice regardless of location.

_Wazzak

[jlb]

Name one.

[cire]

class Base
{
    public:
        virtual void method( int ) ;
        virtual void method( const char*) = 0;
};

class Derived : public Base
{
    public:
        using Base::method; 
        void method( const char *) ;
};

[jlb]

I really don't see the point of your using statement. This code as presented will only compile until you try to actually use these classes.

class Base
{
    public:
        virtual void method( int ) ;
        virtual void method( const char*) = 0;
};

class Derived : public Base
{
    public:
        using Base::method;
        void method( const char *) ;
};


int main()
{
   Derived test;

   return 0;
}

main.cpp|18|undefined reference to `vtable for Derived'|

Jim

[cire]

It compiled fine for you. The linking error is because the functions aren't implemented, and thus the vtable cannot be generated.

class Base
{
    public:
        virtual void method( int ) {}
        virtual void method( const char*)=0 ;
};

class Derived : public Base
{
    public:
        using Base::method; 
        void method( const char *) {}
};

class Derived2 : public Base
{
    public:
        void method(const char*) {} 
};

int main()
{
    Derived Test ;
    Test.method(5) ;

    Derived2 Test2 ;
    Test2.method(5) ; // error
}

[Framework]

jlb wrote:

"I really don't see the point of your using statement."

...then you don't fully understand the usage of "using". When "using" is used inside a class, much like cire's example, it can be used to adjust the level-of-access of a derived member. Consider this example:

struct Base
{
    int Member_; // Public
};

struct Child : Base
{
    using Base::Member_; // "Member_" is still public
};

We can change this code to adjust the level-of-access of "Base::Member_"; like so:

struct Child : Base 
{
    private:
        using Base::Member_; // "Member_" is now private
};

Please, read up on the effects and uses of "using" before labelling it as bad practice.

_Wazzak

[dudeman007]

Thanks everyone for the comments, they are really appreciated. After reading them and listening to the lab tutorial on how they want this, I have made some changes. Still feel some fat can be trimmed, thoughts??

Employee.h

// Preprocessor Directives
#pragma once
#include <string>
using namespace std;
//------------------------------------------------------------------------------------------------------------------
// Define Employee Class
class Employee
{
public:
	// Constructors and Destructor
	Employee(void);
	Employee(string first, string last, char gen, int dep, double salary);
	~Employee(void);

	// Methods to Access Attributes 
	double calculatePay(void);
	void displayEmployee(void);

	// Getters and Setters
	string getFirstName(void);
	void setFirstName(string);
	string getLastName(void);
	void setLastName(string);
	char getGender(void);
	void setGender(char);
	int getDependents(void);
	void setDependents(int);
	double getAnnualSalary(void);
	void setAnnualSalary(double);

private:
	// Declare Data Members
	string firstName;
	string lastName;
	char gender;
	int dependents;
	double annualSalary;
};
//------------------------------------------------------------------------------------------------------------------
// Declare Prototypes
string getFirstName(string);
string getLastName(string);
char getGender(char);
int getDependents(int);
double getAnnualSalary(double);
void displayFirstName(string);
void displayLastName(string);
void displayGender(char);
void displayDependents(int);
void displayAnnualSalary(double);
void DislayApplicationInformation();
void DisplayDivider(string);
void DisplayDivider2(string);
string GetInput (string);
void TerminateApplication();
//------------------------------------------------------------------------------------------------------------------ 

Employee.cpp

// Preprocessor Directives
#include "stdafx.h"
#include "Employee.h"
#include <iostream>
#include <iomanip>
//------------------------------------------------------------------------------------------------------------------
// Default Employee Constructor
Employee::Employee(void)
{
	gender = 'U';
	dependents = 0;
	annualSalary = 20000.0;
	firstName = "not given";
	lastName = "not given";
}
//------------------------------------------------------------------------------------------------------------------
// Employee Deconstructor
Employee::~Employee(void)
{
}
//------------------------------------------------------------------------------------------------------------------
// Multi-Arg Employee Constructor
Employee::Employee(string first, string last, char gen, int dep, double salary)
{
	firstName = first;
	lastName = last;
	gender = gen;
	dependents = dep;
	annualSalary = salary;
}
//------------------------------------------------------------------------------------------------------------------
// Define CalculatePay Function
double Employee::calculatePay(void)
{
	return (annualSalary/52);
}
//------------------------------------------------------------------------------------------------------------------
// DisplayEmployee Function
void Employee::displayEmployee(void)
{
	cout << "Employee Name:\t\t" << firstName << " " <<lastName <<endl;
	cout << "Employee Gender:\t" << gender << endl;
	cout << "Employee Dependents:\t" << dependents << endl;
	cout << "Employee Annual Salary:\t$" <<
		setprecision(2) << showpoint << fixed << annualSalary << "\n";
	cout << "Employee Weekly Pay:\t$" << annualSalary/52 << endl;
}
//------------------------------------------------------------------------------------------------------------------
// Define GetFirstName and SetFirstName
string Employee::getFirstName(void)
{
	return firstName;
}

void Employee::setFirstName(string newFirstName)
{
	firstName = newFirstName;
}
//------------------------------------------------------------------------------------------------------------------
// Define GetLastName and SetLastName
string Employee::getLastName(void)
{
	return lastName;
}

void Employee::setLastName(string newLastName)
{
	lastName = newLastName;
}
//------------------------------------------------------------------------------------------------------------------
// Define GetGender and SetGender
char Employee::getGender(void)
{
	return gender;
}

void Employee::setGender(char newGender)
{
	gender = newGender;
}
//------------------------------------------------------------------------------------------------------------------
// Define GetDependents and SetDependents
int Employee::getDependents(void)
{
	return dependents;
}

void Employee::setDependents(int newDependents)
{
	dependents = newDependents;
}
//------------------------------------------------------------------------------------------------------------------
// Define GetAnnualSalary and SetAnnualSalary
double Employee::getAnnualSalary(void)
{
	return annualSalary;
}

void Employee::setAnnualSalary(double newAnnualSalary)
{
	annualSalary = newAnnualSalary;
}
//------------------------------------------------------------------------------------------------------------------ 

[dudeman007]

3rd file.

Lab2.cpp

// Preprocessor Directives
#include "stdafx.h"
#include "Employee.h"
#include <iostream>
#include <string>
using namespace std;
//------------------------------------------------------------------------------------------------------------------
// Main Function
int main()
{
	// Declare Function Variables
	string tempString;
	char gender;
	int dependents = 0;
	double annualSalary;


	DislayApplicationInformation();

	DisplayDivider("Employee 1");

	Employee employee1;

	string firstName = GetInput("First Name");
	employee1.setFirstName(firstName);

	string lastName = GetInput("Last Name");
	employee1.setLastName(lastName);

	tempString = GetInput("Gender");
	gender = tempString.c_str()[0];
	employee1.setGender(gender);

	tempString = GetInput("Number of Dependents");
	dependents = atoi(tempString.c_str());
	employee1.setDependents(dependents);

	tempString = GetInput("Annual Salary");
	annualSalary = atof(tempString.c_str());
	employee1.setAnnualSalary(annualSalary);

	DisplayDivider2("Employee Information");
	employee1.displayEmployee();
	cout << endl;


	Employee employee2("Mary", "Noia", 'F', 5, 24000.00);
	DisplayDivider("Employee 2");
	DisplayDivider2("Employee Information");
	employee2.displayEmployee();

	TerminateApplication();
	employee1.~Employee();
	employee2.~Employee();
	system("pause");

	return 0;
}

// DisplayApplicationInformation Procedure
void DislayApplicationInformation()
{
	// Display Program Header
	cout << "Welcome to your first Object Oriented Program" << endl;
	cout << "Employee Class CIS247C, Week 2 Lab" << endl;
	cout << "Name: Jim Stevens" << endl;
}
//------------------------------------------------------------------------------------------------------------------
// DisplayDivider Prodedure
void DisplayDivider(string outputTitle)
{
	// Display Divider with Output Title
	cout << "\n***************************" << outputTitle <<"***************************" << endl;
}
//------------------------------------------------------------------------------------------------------------------
// DisplayDivider2 Prodedure
void DisplayDivider2(string outputTitle)
{
	// Display Divider with Output Title
	cout << "\n" << outputTitle << endl;
	cout << "\n________________________________________________________________" << endl;
}
//------------------------------------------------------------------------------------------------------------------
// GetInput Function
string GetInput (string inputType)
{ 
	// Declare Function Varaible
	string input;
	// Prompt User for Input
	cout<<"Please enter your "<< inputType <<": ";
	getline(cin, input);
	return input;
}
//------------------------------------------------------------------------------------------------------------------
// TerminateApplication Procedure
void TerminateApplication()
{
	// Display Termination Message
	cout << "\nThank you for using the Employee Class program\n";
}
//------------------------------------------------------------------------------------------------------------------ 

[Framework]

I can still see a few issues:

● You're not using initialiser-lists inside your constructors. Initialisation is far different that assignment. Note that initialisation is giving a variable or object a value during a declaration context.

● You're overloading the destructor of "Employee" when you don't need to.
● You're using magic constants which, when used by themselves, are not descriptive enough to deduce their usage. Prefer constants (for a single constant) or enumerations (for multiple constants) as these give a name to magic constant.

● Avoid excessive use of "std::ostream::endl( )". Prefer the use of the new-line character. Flush the stream before entering a function and before leaving a function.

● Avoid "using namespace ..." inside headers. This increases the possibility of identifier conflicts.
● (This one isn't bad practice, but it's recommended) It's best to declare variables or objects when you actually need them.

_Wazzak

[Catfish2]

It's best to declare variables or objects when you actually need them.

... because objects call a function (the constructor) when they're instantiated.
If you get out of the function without ever using the object, you wasted a bit of time and memory.

[dudeman007]

Framework, now that I have has some sleep I get the initialiser list. So I changed

// Default Employee Constructor
Employee::Employee(void)  
{
	gender = 'U';
	dependents = 0;
	annualSalary = 20000.0;
	firstName = "not given";
	lastName = "not given";
}

to

// Default Employee Constructor
Employee::Employee(void) : gender('U'), dependents(0), annualSalary(20000.0), firstName("not given"), lastName("not given")
{	
}

This is what you are talking about, correct?

Not sure what you mean about overloading the destructor. It is there even though it is not needed until next weeks assignment. Atleast is what I am told.

Can you give me an example on the magic constats, not sure I follow what you mean.

Have gone back and got rid of the endl, like below:

// DisplayEmployee Function
void Employee::displayEmployee(void)
{
	cout << "Employee Name:\t\t" << firstName << " " << lastName << "\n";
	cout << "Employee Gender:\t" << gender << "\n";
	cout << "Employee Dependents:\t" << dependents << "\n";
	cout << "Employee Annual Salary:\t$" <<
		setprecision(2) << showpoint << fixed << annualSalary << "\n";
	cout << "Employee Weekly Pay:\t$" << annualSalary/52 << "\n";

Working on the namepsace, the Professor wants to see it in there for now. Thnk you for the help and suggestions!

Thanks Cat, I am staring to understand more and more what you guys are saying, I have been making notess to keep with me so I can start doing things better as I write the code. Again I really appreciate your feedback!

[Catfish2]

Minor point: in C++, you don't need to put void in the parameter list of a function that doesn't take parameters. (In good old C, you do.)

Can you give me an example on the magic constats, not sure I follow what you mean.

He's most likely referring to hard-coded values.

while (n < 42) {} // bad style, 42 is a magic constant

const int defaultMaximum = 42;
// good style, we gave it a name,
// and we can change its value from a single place

while (n < defaultMaximum) {}

This becomes relevant when you use defaultMaximum over and over again.
http://en.wikipedia.org/wiki/Magic_number_%28programming%29#Unnamed_numerical_constants

[Framework]

dudeman007 wrote:

"This is what you are talking about, correct?"

Yes.

dudeman007 wrote:

"Not sure what you mean about overloading the destructor. It is there even though it is not needed until next weeks assignment. Atleast is what I am told."

If you were told to overload it but not do anything with it then your hands are pretty much tied. But in your personal projects, overloading the destructor (defining a destructor inside a class overrides the destructor the compiler provides (the trivial destructor)) is only necessary if you want something to happen just before the object leaves its scope.

dudeman007 wrote:

"Can you give me an example on the magic constats, not sure I follow what you mean." (sic)

Magic constants are just real numbers such as 1, 10 and 500. In most cases, the use of magic constants do not provide enough information to justify their presence. For example, the following code contains magic constants. The constants used in the code will probably confuse you.

int SomeFunction( int Value_ )
{
    switch( Value_ )
    {
        case 500:
            // ...

        case 502:
           // ...
    }
}

Anyone who reads the above code will, no doubt, will be confused by the magic constants; what do they mean? why are they there? To overcome this, we use named-constants. An example of a named-constant is as follows:

int const NamedConstant( 10 );

As you can see, instead of the value 10, we have a name which we can refer to.

_Wazzak

[Catfish2]

Also, I know this has to do exclusively with style, but I consider this form to be much nicer:

// Default Employee Constructor
Employee::Employee():
	gender('U'),
	dependents(0),
	annualSalary(20000.0),
	firstName("not given"),
	lastName("not given")
{
}