2
\$\begingroup\$

This is for a class assignment. The objective is to write a program that will take in user input for employee information and print the wage based on hours worked.

Please review my code.

#include <ctime>
#include <iostream>

using namespace std;

int GetSalary(int,int,int,int);
int GetYearsOfService(int);


int main()
{
    int employeeId, hoursWorked, yearJoined, fullTime;

    cout << "Employee ID: ";
    cin >> employeeId;

    cout << "Hours Worked: ";
    cin >> hoursWorked;

    cout << "Year Joined: ";
    cin >> yearJoined;

    cout << "FullTime Status (0 for no, 1 for yes): ";
    cin >> fullTime;

    double salary = GetSalary(employeeId, hoursWorked, yearJoined, fullTime);
    double taxes = salary * 0.15;

    cout << endl << "Empolyee ID: " << employeeId << endl << "\tFull time?: ";

    if (fullTime == 0)
        cout << "No";
    else
        cout << "Yes";

    cout << endl << "\tHours worked: " << hoursWorked << endl << "\tWages (after tax) for the month: $"
         << salary - taxes << endl << "\tAmount withheld for tax: $" << taxes;

    return 0;
}

int GetSalary(int employeeId, int hoursWorked, int yearJoined, int fullTime) {
    double hourlyWage = 14;

    //Get the hourly wage
    int wage = 14;

    if( wage>=50 && wage < 100){
        wage += 1.25;
    }else if( wage < 160){
        wage += 2.0;
    }else{
        wage += 3.0;
    }

    double baseSalary = hourlyWage * hoursWorked;
    int yearsOfService = GetYearsOfService(yearJoined);

    //Check if any additional benefits/bonusses are to be paid
    if (fullTime == 1)
    {
        baseSalary += (35 *yearsOfService);
        baseSalary += 300;
    }
    else {
        if (yearsOfService > 5)
            baseSalary += (35 * yearsOfService);
    }
    return baseSalary;
}

int GetYearsOfService(int yearJoined) {
    time_t curTime = time(NULL);

    int elapsed = curTime / (365 * 24 * 60 * 60);

    return 1970 + elapsed - yearJoined;
}
\$\endgroup\$
5
\$\begingroup\$

My first review in awhile, this is short, sorry. I'm sure others will add more.

  1. Please stop doing using namespace std. Read this.
  2. Using double for representing currency is bad. But since this is an assignment, it probably won't matter much.
  3. Use constant variables to represent magic numbers like .15 and 3.0. Create a const variable named TAX_RATE for instance.
  4. You are returning an int yet salary is of type double?
  5. time_t curTime = time(NULL);

    Use nullptr (C++11).

\$\endgroup\$
  • \$\begingroup\$ 1 isn't a major issue in this case since it's being used just before main is declared, but I agree that it should be moved inside main instead. \$\endgroup\$ – Pharap Oct 31 '14 at 21:41
3
\$\begingroup\$
  • You can define main() lastly, eliminating the need for function prototypes.

  • Try to keep variables as close in scope as possible. You're "listing" declarations at the top of main() instead of declaring/initializing them where they're first used.

    In this case, declare each of these variables in front of the respective cins.

  • You don't need all of these std::endls, which perform flushes:

    cout << endl << "Empolyee ID: " << employeeId << endl << "\tFull time?: ";
    

    Instead, use "\n" to output a newline without a flush:

    cout << "\nEmpolyee ID: " << employeeId << "\n\tFull time?: ";
    

    (Also, "Empolyee" is misspelled)

  • It's unnecessary to have an explicit return 0 at the end of main(). As successful termination is already implied at that point, the compiler should do this return automatically.

  • This simple conditional:

    if (fullTime == 0)
        cout << "No";
    else
        cout << "Yes";
    

    can be shortened to a single-line ternary (?:) statement:

    std::cout << (fullTime == 0) ? "No" : "Yes";
    

    Here, the first part after the ? is for true, and the second part is for false.

  • Measurements of time shouldn't be with an int (a signed type), which can hold negative values. The types for elapsed and the function's return should at least be unsigned int.

\$\endgroup\$
1
\$\begingroup\$

In addition to the other answers (which both make excellent points):

Firstly, try not to mince indent/brace styles, pick one and stick to it. (Doesn't matter too much in an assignment, but your tutor may give you praise for it)

The two lines

baseSalary += (35 *yearsOfService);
baseSalary += 300;

can be folded into the singular

baseSalary += (35 * yearsOfService) + 300;

(also note the spaces around the *, this prevents confusion with pointer dereferencing, though I expect you haven't been taught about those yet)

This if statement:

if (fullTime == 1)
{
    baseSalary += (35 *yearsOfService);
    baseSalary += 300;
}
else {
    if (yearsOfService > 5)
        baseSalary += (35 * yearsOfService);
}

can be reduced to:

if (fullTime == 1)
{
    baseSalary += (35 * yearsOfService) + 300;  
}
else if (yearsOfService > 5)
{
    baseSalary += (35 * yearsOfService);
}

or if you prefer to omit the braces:

if (fullTime == 1)
    baseSalary += (35 * yearsOfService) + 300;  
else if (yearsOfService > 5)
    baseSalary += (35 * yearsOfService);

Also, I could be wrong, but fullTime looks like it should be a true/false value, i.e. a bool

I can't say anything much about the validity of the code since I don't know how the wages are calculated, but I'll assume the calculation is correct.

\$\endgroup\$
0
\$\begingroup\$

This whole block

 //Get the hourly wage
int wage = 14;

if( wage>=50 && wage < 100){
    wage += 1.25;
}else if( wage < 160){
    wage += 2.0;
}else{
    wage += 3.0;
}

is unused, since you don't use wage anywhere else... in addition since wage is 14 you already know the results of all the ifs, so they are doubly useless.

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