7
\$\begingroup\$

This code was inspired by this question: Payroll Program. I saw lots of ways I thought the code could be improved. So much so that I thought it wouldn't be particularly constructive to post all of it as an answer to that question. Essentially, I wanted to basically rewrite the whole thing from scratch.

So I did. But I'm far from a C++ expert, so tell me how to make this even better:

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

const double WORK_WEEK_HOURS = 40;
const double OVERTIME_RATE = 1.5;

struct employee {
    int id;
    double payrate;
    double hours;
};

void payroll();
std::string fetchFileName();
employee fetchEmployee();
double wagesForEmployee(employee e);
bool askUser(std::string s);
std::string employeesToString(std::vector<employee> e);
void displayEmployees(std::vector<employee> e);
void saveEmployeesToFile(std::vector<employee> e);


int main(int argc, const char * argv[]) {
    payroll();
    return 0;
}

void payroll() {
    std::vector<employee> employees;

    do {
        employees.push_back(fetchEmployee());
    } while (askUser("Would you like to enter more employees? (y/n): "));

    if (askUser("Would you like to view the information you entered? (y/n): ")) {
        displayEmployees(employees);
    }

    if (askUser("Would you like to save the information you entered to file? (y/n): ")) {
        saveEmployeesToFile(employees);
    }
}

bool askUser(std::string s) {
    char answer;
    std::cout << s;
    std::cin >> answer;
    return (answer == 'y' || answer == 'Y');
}

employee fetchEmployee() {
    employee e;
    e.id = -1;
    e.payrate = -1;
    e.hours = -1;

    do {
        std::cout << "Enter the employee ID: ";
        std::cin >> e.id;
    } while(e.id < 0);

    do {
        std::cout << "Enter the payrate: ";
        std::cin >> e.payrate;
    } while (e.payrate < 0);

    do {
        std::cout << "Enter the hours worked: ";
        std::cin >> e.hours;
    } while (e.hours < 0);

    return e;
}

double wagesForEmployee(employee e) {
    if (e.hours < 0 || e.payrate < 0) {
        return 0;
    }
    double wages = 0;
    if (e.hours > WORK_WEEK_HOURS) {
        wages += (e.hours - WORK_WEEK_HOURS) * OVERTIME_RATE * e.payrate;
    }
    wages += std::min(e.hours, WORK_WEEK_HOURS) * e.payrate;
    return wages > 0 ? wages : 0;
}

std::string employeesToString(std::vector<employee> e) {
    std::stringstream ss;

    ss << std::setw(10) << "ID" << std::setw(10) << "Rate" << std::setw(10) << "Hours" << std::setw(10) << "Wage" << std::endl;
    ss << "---------------------------------------------------------" << std::endl;

    for (int i = 0; i < e.size(); ++i) {
        employee curEmp = e.at(i);
        ss  << std::setw(10) << curEmp.id << std::setprecision(2) << std::fixed
            << std::setw(10) << curEmp.payrate << std::setprecision(2) << std::fixed
            << std::setw(10) << curEmp.hours << std::setprecision(2) << std::fixed
            << std::setw(10) <<  wagesForEmployee(curEmp) << std::endl;
    }

    return ss.str();
}

void displayEmployees(std::vector<employee> e) {
    std::cout << employeesToString(e) << std::endl;
}

void saveEmployeesToFile(std::vector<employee> e) {
    std::ofstream outputFile;
    std::string fileName;

    std::cout << "Enter a file name: ";
    // TODO: Safety checking for illegal file name characters, etc.
    std::cin >> fileName;
    fileName += ".pay";
    outputFile.open(fileName.c_str());
    outputFile << employeesToString(e);
    outputFile.close();

    std::cout << "Successfully saved to " << fileName;
}
\$\endgroup\$

2 Answers 2

9
\$\begingroup\$
double wagesForEmployee(employee e);
bool askUser(std::string s);
std::string employeesToString(std::vector<employee> e);
void displayEmployees(std::vector<employee> e);
void saveEmployeesToFile(std::vector<employee> e);

When you're only reading from a variable and not modifying it, you should take it by const reference. Taking it by value like this means that you copy it.

This is particularly bad in displayEmployees since it makes a copy of the entire employee vector then passing that copy to employeesToString which then makes another copy.


e.id = -1;
e.payrate = -1;
e.hours = -1;

do {
    std::cout << "Enter the employee ID: ";
    std::cin >> e.id;
} while(e.id < 0);

Rather interestingly, you've manged to stumble across a backwards-compatibility breaking change in C++11. As per cppreference:

Before C++11:

If extraction fails (e.g. if a letter was entered where a digit is expected), value is left unmodified and failbit is set.

Since C++11:

If extraction fails, zero is written to value and failbit is set.

This means that compiled under C++11 or newer, this code is wrong. A better approach is to simply check the result of the extraction directly:

do {
    std::cout << "Enter the employee ID: ";
} while(!(std::cin >> e.id));

Note that this has the behavior of checking whitespace delimited words. In other words, if you want to skip the rest of the line on erroneous input, you'll need to do something likely involving std::getline to eat the rest of the line. As it is, entering something like a b c 5 would output the prompt text 4 times but only actually allow the user to provide input once. (And it would exit the loop with e.id == 5.)


int main(int argc, const char * argv[]) {
    payroll();
    return 0;
}
  1. I'd probably just pull payroll up into main since main doesn't do anything else.
  2. Omitting return 0 silently inserts return 0. In other words, when a program only exits with a success status, you can omit the return statement so that people reading the code can see that at a glance.
  3. If you don't use argc and argv, I wouldn't bother to take them. int main() is just as valid.

double wagesForEmployee(employee e) {
    // #1
    if (e.hours < 0 || e.payrate < 0) {
        return 0;
    }
    double wages = 0;
    // #2
    if (e.hours > WORK_WEEK_HOURS) {
        wages += (e.hours - WORK_WEEK_HOURS) * OVERTIME_RATE * e.payrate;
    }
    // #2
    wages += std::min(e.hours, WORK_WEEK_HOURS) * e.payrate;
    // #3
    return wages > 0 ? wages : 0;
}
  1. If you're going to start applying business rules to employee, I'd put them in the employee class. This would unfortunately complicate complicate the code to prompt for employee information on the command line though.
  2. Super minor and completely opinion: I would calculate normal wages and then add overtime on top. It threw me for a second to realize that overtime was being done first.
  3. When wages >= 0, this is a (logical) no-op. That means that you're verifying that wages are positive. But, earlier in the function, you've verified that e.hours is nonnegative and payrate is nonnegative. OVERTIME_RATE is also presuambly nonnegative. In other words, why is this check here?

As a general note: don't hide error conditions. 0 is presumably a valid return from this function, so using it as an error sentinel is throwing away information. Instead, you need to either assert or throw an exception when you get unexpected inputs. Which you do depends on if you consider invalid inputs programmer error or user error. Personally, I would use assert here since I would expect the consuming programmer to provide a sane employee. (Actually I wouldn't check it at all, but some might say I err too far on the side of anti-precondition-verification.)

Another general note: this is a function that is oriented around an employee instance. It's also deeply aware of the internals of an employee object, and it's coupled to it. You see where I'm going with this, but just in case: this should be a method on the employee class. Stop seeing employee as a data bag, and start seeing it as a class. The e.hours >= 0 and e.payrate >= 0 conditions can be handled on the class, and this method would fit naturally onto it. You could even implement operator<<(ostream&) on it to simply printing it (though personally I have mixed feelings on human aimed operator<<(ostream&) implementations).


With regards to your employee outputting, you should have a function to output a single employee and then loop over that.

Also, Converting employees to a string and then outputting that string is incredibly inefficient. A better approach in C++ than building strings and outputting them is to output to streams and then build a string from that if you so desire. For example:

std::ostream& outputEmployee(ostream& os, const employee& e) {

    // You would of course want to do proper formatting -- just a lazy example.
    // Note that a new line is not included. That is because the newline implies some form of looping is happening, but this function operates on a single employee.
    os << e.id << ' ' << e.payrate << ' ' << e.hours;

    return os; // So the caller can easily check success ala if (outputEmployee(os, emp)) { ... }

    // In a real function, I would be tempted to put the return in the same line as the stream writing.
    // That has the same effect, but it saves a useless `return os;` line.
    // Example: return os << e.id << ... ;

}

std::string employeeToString(const employee& e) {
    std::ostringstream oss;
    // If you wanted to be really paranoid, you could check oss after this call.
    // Nothing should (reasonably) fail though, so meh.
    outputEmployee(oss);
    return oss.str();
}

This is pretty much the exact same process you wrote, but the overhead of buffering into a string only to dump out to a stream is avoided.

As mentioned earlier, you could override operator<<(ostream&). That would have the advantage of being a tiny bit cleaner to call (std::cout << emp << '\n'), and interestingly enough, it would also allow you to use some standard library tricks. For example, if you wanted to loop over outputting a collection of employees:

std::copy(std::begin(employees), std::end(employees),
          std::ostream_iterator<employee>(std::cout, "\n"));

Reading employees in can be done in a similar way if operator>> is overloaded:

std::vector<employee> employees;
std::copy(std::istream_iterator<employee>(std::cout), 
          std::istream_iterator<employee>(),
          employees);

As mentioned in passing earlier though, human aimed >> and << make me a little uncomfortable (at least when the operators are defined directly on the class and not some kind of formatting decorator). I more of see them as debugging methods or methods for automatic reading (like parsing in a file or something instead of prompting a user).


I don't like how much saveEmployeesToFile does. I would pull the prompting for file name and opening of the file into at least 1 function and have saveEmployeesToFile take an ostream&. Happy side effect: you can then easily support outputting to std::cout if you want to pass that instead of a file stream.


With regards to saveEmployeesToFile:

  • Provided that functions stay small and focused, std::ofstream::close() is mostly noise. The file will be closed on destruction anyway.

  • If you prompt for the file name before you define the std::ofstream instance, you can avoid the open call.

  • You should verify that the file was successfully opened.


#includes should be alphabetized (though grouped by source first -- local includes first then 3rd party libraries and the standard library).

\$\endgroup\$
6
\$\begingroup\$

One thing I noticed is some of your variable names don't mean very much - e, s, and ss, for example. While your methods are short and easy to follow, it is always a good idea to give variables meaningful names.

Another thing I noticed is that you may want to provide error handling here, and in similar areas:

do {
    std::cout << "Enter the employee ID: ";
    std::cin >> e.id;
} while(e.id < 0);

If the user were to enter "r" instead of an int value, for instance, it would crash. You can fix this by adding this:

do {
    std::cout << "Enter the employee ID: ";
    std::cin >> e.id;

    if (!std::cin) {
        std::cin.clear();
        std::string s = "";
        getline(std::cin, s);
    }
} while(e.id < 0);

This still will not handle the user inputting a double value. To do that, you could input a double and compare the floor (you would need to #include <cmath>) of the input to the input and determine whether the input is a valid value.

\$\endgroup\$
4
  • \$\begingroup\$ Yeah. Given that essentially all of my programming experience is UI-based applications, I'm extraordinarily lazy about handling command line user input. \$\endgroup\$
    – nhgrif
    Commented Feb 19, 2015 at 0:29
  • \$\begingroup\$ Error handling should definitely be added (+1), but unfortunately it's a bit more complicated than this. For example, try putting in somestring as input. That will of course fail and set the failbit, but e.id will be set to 0 (in C++11 -- before C++11 the value will be unchanged). That means that the loop will actually exit if reading fails since e.id will be 0 (or, before C++11, it's indeterminate, but there's a good chance it will continue past the loop). The loop should either be controlled by the failbit, or a different approach will need to be taken. \$\endgroup\$
    – Corbin
    Commented Feb 19, 2015 at 2:43
  • \$\begingroup\$ Also, while I'm at it, std::cin >> s will only clear 1 word, not the rest of the line. That might be desired behavior, but in the case that it's not, you would need to use std::getline instead. \$\endgroup\$
    – Corbin
    Commented Feb 19, 2015 at 2:44
  • \$\begingroup\$ Thanks for the comments, @Corbin. Will update my answer when I'm not using my phone later. \$\endgroup\$
    – user34073
    Commented Feb 19, 2015 at 4:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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