Printing/Saving a Payroll Chart

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
#include
#include
#include
#include
#include
#include

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 e);
void displayEmployees(std::vector e);
void saveEmployeesToFile(std::vector e);

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

void payroll() {
std::vector 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 > answer;
return (answer == 'y' || answer == 'Y');
}

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

do {
std::cout > e.id;
} while(e.id > e.payrate;
} while (e.payrate > e.hours;
} while (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 e) {
std::stringstream ss;

ss

double wagesForEmployee(employee e);
bool askUser(std::string s);
std::string employeesToString(std::vector e);
void displayEmployees(std::vector e);
void saveEmployeesToFile(std::vector 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 > 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 > 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;
}

• I'd probably just pull payroll up into main since main doesn't do anything else.

• 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.

• 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  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;
}

• 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.

• 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.

• 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> is overloaded:

std::vector employees;
std::copy(std::istream_iterator(std::cout), 
          std::istream_iterator(),
          employees);

As mentioned in passing earlier though, human aimed >> and

-
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.

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

StackExchange Code Review Q#81876, answer score: 9