8
\$\begingroup\$

I wrote a program that calculates gross wages for employees. The program asks the user enter an employee's ID, hours worked and hourly pay rate. It then calculate the gross wage for that employee and store this employee’s information into a file, chosen by the user. It then asks the user if they would like to show the information that has been entered.

I would really appreciate any help from fellow peers on any pointers on how I could improve my program.

#include <cstdlib>
#include <iostream>
#include <fstream>
#include <string>
#include <iomanip>

using namespace std;

// Prototype functions
double calcWage ();
void getEmployeeInfor();
void printWages();
void printTable();
void repeat();
void read();

// Defining terms for the program.
int id,result;
double payRate, hours, wage;
ofstream outputFile;
ifstream inputFile;
string file,strings;

int main() 
{

   cout << "Please enter the name of the file you wish to save to: ";
   cin >> file;
   outputFile.open(file.c_str());

   // Get employee information
   getEmployeeInfor();
   // Calculate the total wage
   wage = calcWage();
   // Prints the Table to the outputFile
   printTable();   
   // Prints the  wages to the output File
   printWages();
   // Ask if more informations wants to be added/ calculated
   repeat();
   // Call read function to read information to console
   read ();

 return 0;

}

// Function for asking user for Employee Id, pay rate, and the hours worked
void getEmployeeInfor()

{
 cout << "Please enter your employee id, payrate, and hours worked: " << endl;
 cin >> id >> payRate >> hours;

 //Loop to check for valid numbers.
   while ((id < 0) || payRate < 0 || hours < 0)
  {
   cout << "Please enter valid values (Positive numbers): " << endl;
   cin >> id >> payRate >> hours;
  }
}

// Function for calculating the Wage
double calcWage()
{
  double wages;
  return wages = payRate * hours;
}

// Function for printing information to the outputFile
void printWages()
{
  outputFile << setw(10) << id 
           << setprecision(2) << fixed <<  setw (10)
           << payRate
           << setprecision(2) << fixed <<  setw (10)
           << hours
           << setprecision(2) << fixed <<  setw (10)
           <<  wage << endl;
} 

// Function to print table to the outputFile
void printTable()

{
  outputFile << setw(10) << "ID" << setw (10) << "Rate" << setw(10) << "Hours"
             << setw(10) << "Wage"<<endl;
  outputFile << "---------------------------------------------------------\n";
}

// Function that asks the user if the process wants to be repeated
void repeat()
{  
  char answer;
  cout << "Would you like to continue entering values? (y/n): ";
  cin >> answer;
  if (answer == 'y'|| answer == 'Y')
  {
    getEmployeeInfor();   // gets employee information
    wage = calcWage();    // calculates wage
    printWages();         // prints wage to the outputfile
    // calls repeat function again;
     repeat();
   }
  else
   {
    // Close the outputfile
    outputFile.close();
    cout << file << " was closed succesfully" << endl;
   }
}

// Function that reads the data to the console
void read()
 {
   char repeatAnswer;
   cout << "Would you like to read the file? (y/n): ";
   cin  >> repeatrAnswer; 
   if (repeatrAnswer == 'y' || repeatrAnswer == 'Y')
    {
        inputFile.open(file.c_str());
        while (!inputFile.eof())
         {
           getline(inputFile, strings);
           cout << strings<< endl;
         }
    cout << "Reading " << file << " completed." << endl;
    }

  // Close inputFile
  inputFile.close();
  // Terminate the program
  exit(0);
 }
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! Would you please edit your title and question to explain what your code does, rather than just asking for improvements? \$\endgroup\$ – Phrancis Feb 18 '15 at 20:42
  • 1
    \$\begingroup\$ you tag it C++, but where are the objects? Your code basically looks like a C-program. Identify the objects, make classes and think OO \$\endgroup\$ – AndersK Feb 18 '15 at 21:49
  • \$\begingroup\$ For a learning program it's not too much of a problem, just be aware that doubles (like all binary floating point) can't store some decimal values precisely. Take .1 for example: .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 != 1.0 (it's .999999... something or other). You need an equivalent to C#'s decimal (or Java's BigDecimal), although I'm not sure if C/C++ has anything natively. Accountants start to get worried when money disappears this way... \$\endgroup\$ – Clockwork-Muse Feb 19 '15 at 12:56
  • \$\begingroup\$ Not an answer you wanted, but practically speaking it would be more useful to read a CSV file (or similar) for raw input. It scales much better. \$\endgroup\$ – Owen Beresford Feb 19 '15 at 13:18
10
\$\begingroup\$

First, you are using namespace std;. This is not usually a good idea because if you use more than one namespace, you may have functions that have the same name, and the compiler will not know which to use. More information can be found here.

Another potential problem is that you have a fair amount of global variables. You should keep your variables to as tight a scope as possible, so I would recommend creating at least some of these in main() and passing them to other functions as needed.

getEmployeeInfor(); should be getEmployeeInfo(); as info is the accepted abbreviation for information (it even passes my spell check).

You are using redundant comments:

// Get employee information
getEmployeeInfor();

Your naming is very good, and you don't need to say in a comment what you are clearly saying in your code.

Your indentation is a bit off:

void getEmployeeInfor()

{
 cout << "Please enter your employee id, payrate, and hours worked: " << endl;
 cin >> id >> payRate >> hours;

 //Loop to check for valid numbers.
   while ((id < 0) || payRate < 0 || hours < 0)
  {
   cout << "Please enter valid values (Positive numbers): " << endl;
   cin >> id >> payRate >> hours;
  }
}

The standard indentation (usually) is 4 spaces. I would write this like this:

void getEmployeeInfo()
{
    std::cout << "Please enter your employee id, pay rate, and hours worked:\n";
    std::cin >> id >> payRate >> hours;

    //Loop to check for valid numbers.
    while (id < 0 || payRate < 0 || hours < 0)
    {
        std::cout << "Please enter valid values (Positive numbers):\n";
        std::cin >> id >> payRate >> hours;
    }
}

I cleaned the indentation and spacing up, I removed unnecessary braces from the while conditions, I specified that I am using the std namespace, and I changed the << endl section in the output to the newline character \n because you are using a string anyway.

Most of your other methods have indentation problems, but you should be able to clean them up using these tips.

\$\endgroup\$
6
\$\begingroup\$
void repeat()
{  
  char answer;
  cout << "Would you like to continue entering values? (y/n): ";
  cin >> answer;
  if (answer == 'y'|| answer == 'Y')
  {
    getEmployeeInfor();   // gets employee information
    wage = calcWage();    // calculates wage
    printWages();         // prints wage to the outputfile
    // calls repeat function again;
     repeat();
   }
  else
   {
    // Close the outputfile
    outputFile.close();
    cout << file << " was closed succesfully" << endl;
   }
}

This approach is extraordinarily problematic.

Open up any program that can report on the amount of memory your system's processes are consuming and start observing how much RAM is required by your application as you repeatedly answer y to the question your repeat() function asks.

This is unintended recursion you've got going on here. Each time we repeat(), our program consumes more and more RAM because we have to add another copy of all of these functions and all of their local variables to the stack. Eventually, depending on how much RAM your machine has and how bored you are, you can get this program to run into a Stack Overflow exception.

This isn't a memory leak. All of the memory will be appropriate released. It's not trapped anywhere and forgotten about. It's just that the application is designed in such a way that it consumes far more memory than it needs.

Moreover, this approach results in some code duplication.

Instead, perhaps we need a while or dowhile loop? Let's change the repeat() function to return a bool and simply determine whether or not the user wants to continue without actually doing anything:

bool repeat()
{
    char answer;
    cout << "Would you like to continue entering values? (y/n): ";
    cin >> answer;
    return (answer == 'y' || answer == 'Y');
}

Now let's rewrite main(), shall we?

int main() 
{
    cout << "Please enter the name of the file you wish to save to: ";
    cin >> file;
    outputFile.open(file.c_str());

    do {
       // Get employee information
       getEmployeeInfor();
       // Calculate the total wage
       wage = calcWage();
       // Prints the Table to the outputFile
       printTable();   
       // Prints the  wages to the output File
       printWages();
    } while(repeat());

    // Close the outputfile
    outputFile.close();
    cout << file << " was closed succesfully" << endl;

    read();

    return 0;
}

This has the added benefit of closing the file in the same context in which it is opened (it's a massive red-flag otherwise--though we should really only open just before we need to read/write and close immediately after we've finished). Moreover, the flow of the program makes a lot more sense now. And we've eliminated some duplicated code... and that nasty unseen RAM problem.

\$\endgroup\$
  • \$\begingroup\$ Well actually all he uses are global variables, so there is no memory problem :) Good point though, since global variables should not be used, and fixing that would have lead to the problem you point out. \$\endgroup\$ – Loufylouf Feb 19 '15 at 0:06
  • \$\begingroup\$ @Loufylouf He doesn't only use global variables. The repeat() function declares a char and the calcWage() function declares a double. Moreover, I'm pretty sure the function call itself consumes some memory, right? Something is keeping track of the stack. \$\endgroup\$ – nhgrif Feb 19 '15 at 0:09
  • \$\begingroup\$ Yeah you're right, my bad. \$\endgroup\$ – Loufylouf Feb 19 '15 at 0:12
  • \$\begingroup\$ Thank you, for your pointer on how I can make my program more efficient. \$\endgroup\$ – Daniel Kajewski Feb 19 '15 at 0:31
  • \$\begingroup\$ @DanielKajewski codereview.stackexchange.com/questions/81876/… I rewrote your program from scratch-ish. \$\endgroup\$ – nhgrif Feb 19 '15 at 0:32
5
\$\begingroup\$
// Function for calculating the Wage
double calcWage()
{
  double wages;
  return wages = payRate * hours;
}

There's no point in declaring a variable and assigning to it:

double calcWage() {
    return payRate * hours;
}

Or even better:

double calcWage(double payRate, double hours) {
    return payRate * hours;
}

You have basically no error checking. File opening, stream token extraction, and file closing can all fail.

In fact:

outputFile.close();
cout << file << " was closed succesfully" << endl;

That's a lie. If close() fails, it sets the failbit but otherwise does nothing. It's completely possible that flushing out some cached stuff failed and the the full content of the file didn't make it to disk.


    inputFile.open(file.c_str());
    while (!inputFile.eof())
     {
       getline(inputFile, strings);
       cout << strings<< endl;
     }

This is wrong. It will actually 'read' an extra line erroneously since eof() doesn't return true until an attempt has been made to read past the end of the stream. The idiomatic way to do this would be:

for (std::string line; std::getline(inputFile, line); ) {
    std::cout << line << '\n';
}
// Maybe followed by a std::cout << std::flush depending on situation

Or, better yet, there's no point to read the stream line by line just to output it line by line again. You can just copy the stream to std::cout with std::cout << inputFile.rdbuf();. (Note that this does have very slightly different semantics since it's a binary copy instead of a character stream.)


Functions should be responsible for one discrete responsibility, but this is broken in a few places. For example, read(): 1) prompts for user input, 2) opens a file, 3) copies the file to cout, 4) closes the file, 5) terminates the program.

That's a lot of things for one function to be doing. Main should handle the exiting (since it's responsibility is to oversee the flow of the program execution), and instead of opening and closing a file, read should be passed an already open file.


strings has no reason to be global, and it should be called line, especially since it's a single string, not a collection of them.


inputFile should not be global. It's not necessary for it to global since every function that uses it immediately closes it. Even better, since you're not actually explicitly checking if the file closes successfully (and really, it'd be fairly overkill to do that unless you're making a very robust program), if you use a local variable, it will close itself automatically.

void doSomethingWithAFile() {
    std::ofstream os("some file name");
    // Do something with the file
} // File automatically closes when os is destructed

Side note: This is actually a veiled peak at RAII, or the concept that resource life times should be tied to object life times (over simplified, but yeah). It's probably a bit early at this point to go reading into that too much, but once you're deeper into C++ (perhaps when you find yourself becoming fairly comfortable with the object model), make sure you look it up at some point.


This has already been said before by someone else, but just to reiterate, don't use globals. Just don't. At least not until you come across your first situation where you truly cannot do anything else (and it's possible to go years in between those situations). To be a bit dramatic, globals go against pretty much every software design principle there is.

Your program can very easily eliminate globals by using local variables in main and passing parameters to functions as needed. I know in an example program this simple and small it doesn't seem like it really makes a difference (it might even seem more cumbersome), but it really is a habit you should get out of. In large or complex systems, globals can quickly turn into a a testability killer and become Petri dishes for weird, hard to find bugs.

(Note: immutable globals [usually called constants] don't have nearly as big of set of problems [though they still have their own set of problems]. If you actually heed my end-of-the-world-warning about globals, just know that you can chill out a bit towards constants as long as you're sure to be mindful of if/when they become determinant to the quality of the application.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.