Is there a better, more efficient way to write this program?

Discussion in 'C++' started by brian.digipimp@gmail.com, Nov 10, 2005.

  1. Guest

    I turned this in for my programming fundamentals class for our second
    exam. I am a c++ newb, this is my first class I've taken. I got a good
    grade on this project I'm just wondering if there is a better and more
    efficient way to write this program.

    //This program calculates the users Gross Pay and subtracts the tax
    based off of Marital Status
    //and the number of exemptions they select

    #include <iostream>
    #include <iomanip>
    #include <string>
    #include <stdlib.h>
    using namespace std;


    int main()
    {

    string FirstName; //Holds the user inputed First Name
    string LastName; //Holds the user inputed Last Name
    char MaritalStatus; //Holds the user inputed value for Marital
    Status
    double HoursWorked; //Holds the user inputed value of Hours Worked
    double HourlyRate; //Hodls the user inputed value of Hourly Rate
    double GrossPay; //Calculates and holds the users inputed value for
    Gross Pay
    double FederalTax; //Holds the calculations for calculating
    FederalTax
    double StateTax; //Holds the calculations for calculating StateTax
    int NumExemptions; //Holds the user inputed value for Number of
    Exemptions


    cout << "Enter your first name:";
    cin >> FirstName;
    cout << "Enter your last name:";
    cin >> LastName;
    cout << "Input the amount of hours worked:";
    cin >> HoursWorked;
    cout << "Input the hourly rate:";
    cin >> HourlyRate;
    cout << "Input the marital status ('M' for married and 'S' for
    single): ";
    cin >> MaritalStatus;
    GrossPay = HoursWorked * HourlyRate;
    StateTax = GrossPay * .0825;

    switch (MaritalStatus)
    {
    case 'S':
    cout << "Input the number of exemptions:";
    cin >> NumExemptions;
    cout << system("cls");
    cout << "Payroll information for ";
    cout << LastName << ", ";
    cout << FirstName << ":";
    cout << endl;
    cout << endl;
    cout << "Hours Worked: ";
    cout << setw(24);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << HoursWorked;
    cout << endl;
    cout << "Hourly Rate: ";
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << HourlyRate;
    cout << endl;
    cout << "Marital Status: ";
    cout << setw(22);
    cout << "Single";
    cout << endl;
    cout << "Number of Exemptions: ";
    cout << setw(16);
    cout << NumExemptions;
    cout << endl;
    cout << endl;
    cout << "Gross Pay";
    cout << setw(29);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << GrossPay;
    cout << endl;
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << "Federal Taxes";


    if (NumExemptions == 0)
    {
    cout << setw(25);
    cout << (GrossPay * .179);
    FederalTax = GrossPay * .179;
    }

    else if (NumExemptions == 1)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .168);
    FederalTax = GrossPay * .168;
    }

    else if (NumExemptions == 2)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .157);
    FederalTax = GrossPay * .157;
    }

    else if (NumExemptions == 3)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .146);
    FederalTax = GrossPay * .146;
    }

    else if (NumExemptions >= 4)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .125);
    FederalTax = GrossPay * .125;
    }

    cout << endl;
    cout << "State Taxes";
    cout << setw(27);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << GrossPay * .0825;
    cout << endl;
    cout << endl;
    cout << "Net Pay";
    cout << setw(31);
    cout << (GrossPay - FederalTax - StateTax);
    cout << endl;

    break;

    case 'M':
    cout << "Input the number of exemptions:";
    cin >> NumExemptions;
    cout << system("cls");
    cout << "Payroll information for ";
    cout << LastName << ", ";
    cout << FirstName << ":";
    cout << endl;
    cout << endl;
    cout << "Hours Worked: ";
    cout << setw(24);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << HoursWorked;
    cout << endl;
    cout << "Hourly Rate: ";
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << HourlyRate;
    cout << endl;
    cout << "Marital Status: ";
    cout << setw(22);
    cout << "Married";
    cout << endl;
    cout << "Number of Exemptions: ";
    cout << setw(16);
    cout << NumExemptions;
    cout << endl;
    cout << endl;
    cout << "Gross Pay";
    cout << setw(29);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << GrossPay;
    cout << endl;
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << "Federal Taxes";

    if (NumExemptions == 0)
    {
    cout << setw(25);
    cout << (GrossPay * .151);
    FederalTax = GrossPay * .151;
    }

    else if (NumExemptions == 1)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .142);
    FederalTax = GrossPay * .142;
    }

    else if (NumExemptions == 2)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .133);
    FederalTax = GrossPay * .133;
    }

    else if (NumExemptions == 3)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .124);
    FederalTax = GrossPay * .124;
    }

    else if (NumExemptions >= 4)
    {
    cout << setw(25);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << (GrossPay * .105);
    FederalTax = GrossPay * .105;
    }

    cout << endl;
    cout << "State Taxes";
    cout << setw(27);
    cout << GrossPay * .0825;
    cout << endl;
    cout << endl;
    cout << "Net Pay";
    cout << setw(31);
    cout << GrossPay - FederalTax - StateTax;
    cout << endl;

    break;

    default: cout << "Invalid Status, Please type either 'M' for married
    or 'S' for single";
    }
    return 0;
    }
    , Nov 10, 2005
    #1
    1. Advertising

  2. wrote:
    > I turned this in for my programming fundamentals class for our second
    > exam. I am a c++ newb, this is my first class I've taken. I got a good
    > grade on this project I'm just wondering if there is a better and more
    > efficient way to write this program.
    >


    Well define efficient, and explain why you are concerned about it.
    Computers goes very fast you know, efficiency should be the absolute
    least of your concerns. Right at the very, very bottom of the pile.

    The single biggest improvement you could make would be to split the code
    up into smaller functions instead of having everything in main. For
    instance you could start with a process_single_person function and a
    process_married_person function. This improvement has nothing to do with
    efficiency, its about writing understandable, maintainable code. But
    maybe you haven't covered writing functions yet.

    john
    John Harrison, Nov 10, 2005
    #2
    1. Advertising

  3. Guest

    Functions are actually in the next chapter. So the only thing I've done
    with them was the prime numbers program last night. Im not really
    concerned with the efficiency speed wise I guess, but code wise. Could
    I have done it with less code just as well?
    , Nov 10, 2005
    #3
  4. Guest

    wrote:
    > I turned this in for my programming fundamentals class for our second
    > exam. I am a c++ newb, this is my first class I've taken. I got a good
    > grade on this project I'm just wondering if there is a better and more
    > efficient way to write this program.


    More efficient is relative. If you mean coding and maintenacne time,
    then yes!
    See my comments below.

    > //This program calculates the users Gross Pay and subtracts the tax
    > based off of Marital Status
    > //and the number of exemptions they select
    >
    > #include <iostream>
    > #include <iomanip>
    > #include <string>
    > #include <stdlib.h>


    What do you need stdlib.h for system() (system("cls") is not portable)?
    Even if you needed it, wouldn't cstdlib be more appropriate?

    > using namespace std;
    >
    >
    > int main()
    > {
    >
    > string FirstName; //Holds the user inputed First Name


    When your variable names are as expressive as they are in this case,
    the comment is very redundant and not needed. Same for the rest of the
    variable declarations.

    > string LastName; //Holds the user inputed Last Name
    > char MaritalStatus; //Holds the user inputed value for Marital
    > Status
    > double HoursWorked; //Holds the user inputed value of Hours Worked
    > double HourlyRate; //Hodls the user inputed value of Hourly Rate


    "Holds"...

    > double GrossPay; //Calculates and holds the users inputed value for
    > Gross Pay
    > double FederalTax; //Holds the calculations for calculating
    > FederalTax
    > double StateTax; //Holds the calculations for calculating StateTax
    > int NumExemptions; //Holds the user inputed value for Number of
    > Exemptions
    >
    >
    > cout << "Enter your first name:";
    > cin >> FirstName;
    > cout << "Enter your last name:";
    > cin >> LastName;
    > cout << "Input the amount of hours worked:";
    > cin >> HoursWorked;
    > cout << "Input the hourly rate:";
    > cin >> HourlyRate;
    > cout << "Input the marital status ('M' for married and 'S' for
    > single): ";
    > cin >> MaritalStatus;
    >
    > GrossPay = HoursWorked * HourlyRate;
    > StateTax = GrossPay * .0825;


    Magic number here. Use a constant instead to make this more expressive.

    static const double StateTaxRate = 0.0825;
    ...
    StateTax = GrossPay * StateTaxRate;

    >
    > switch (MaritalStatus)
    > {
    > case 'S':
    > cout << "Input the number of exemptions:";
    > cin >> NumExemptions;
    > cout << system("cls");
    > cout << "Payroll information for ";
    > cout << LastName << ", ";
    > cout << FirstName << ":";


    This output code is very reduntant. You are doing this for both M and
    S. So it would be much better to get it outside of the switch loop or
    make it a a function. Use a ternary operator or similar where there are
    slight differences between M and S.

    > cout << endl;
    > cout << endl;
    > cout << "Hours Worked: ";
    > cout << setw(24);
    > cout << fixed;
    > cout << showpoint;
    > cout << setprecision(2);
    > cout << HoursWorked;
    > cout << endl;


    Combine these into less lines:

    cout << "Hours Worked: ";
    cout << setw(24) << fixed << showpoint << setprecision(2) <<
    HoursWorked << endl;

    [more output code snipped...]

    > if (NumExemptions == 0)
    > {
    > cout << setw(25);
    > cout << (GrossPay * .179);
    > FederalTax = GrossPay * .179;
    > }


    Magic numbers - see above...

    > else if (NumExemptions == 1)
    > {
    > cout << setw(25);
    > cout << fixed;
    > cout << showpoint;
    > cout << setprecision(2);
    > cout << (GrossPay * .168);
    > FederalTax = GrossPay * .168;
    > }


    Once again you're repeating the output code many times. Why not simply
    detemine the rate in a switch() and then output it at once. Kind of
    like so (minus the magic numbers):

    double FederalTaxRate = 0;

    switch( NumExemptions )
    {
    case 0: FederalTaxRate = 0.179; break;
    case 1: FederalTaxRate = 0.168; break;
    case 2: FederalTaxRate = 0.157; break;
    case 2: FederalTaxRate = 0.146; break;
    default: FederalTaxRate = 0.125; break;
    }

    FederalTax = GrossPay * FederalTaxRate;

    cout << setw(25) << fixed << showpoint << setprecision(2) <<
    FederalTax << endl;

    [mode eval code snipped]

    > default: cout << "Invalid Status, Please type either 'M' for married
    > or 'S' for single";
    > }


    It would be nicer to check for this at input time, so that one doesn't
    have to re-enter all the other numbers just because he/she made a typo
    at the marrital status.

    Overall, you could easily trim 3/4 of your program and make it a lot
    more readable without any loss of functionality.

    As a general rule, everytime when you write the same code twice in a
    row somewhere (or copy and paste a large block), you should ask
    yourself "Why am I doing this?" and see if there's similarities between
    the repeating blocks that you can streamline or combine into a
    function.

    Cheers,
    Andre
    , Nov 10, 2005
    #4
  5. Brian Guest

    I like those suggestions, thanks Andre. haha I like your comment about
    my variable comments. I just copy and pasted all of my code, my
    professor makes us comment every variable and explain them. If this
    were done outside of my class I probably wouldn't have commented that.
    :)
    Brian, Nov 10, 2005
    #5
  6. Neil Cerutti Guest

    On 2005-11-10, Brian <> wrote:
    > I like those suggestions, thanks Andre. haha I like your comment about
    > my variable comments. I just copy and pasted all of my code, my
    > professor makes us comment every variable and explain them. If this
    > were done outside of my class I probably wouldn't have commented that.
    >:)


    It's a OK habit whilst you're learning, I guess.

    If you're interested in good, professional programming style,
    check out _The Practice of Programming_ by Kernighan & Pike. The
    first chapter is full of good advice on naming your variables,
    and the right way to use comments.

    --
    Neil Cerutti
    Neil Cerutti, Nov 10, 2005
    #6
  7. wrote:
    > Functions are actually in the next chapter. So the only thing I've done
    > with them was the prime numbers program last night. Im not really
    > concerned with the efficiency speed wise I guess, but code wise. Could
    > I have done it with less code just as well?
    >


    Well you could have chained your use of cout

    For instance

    cout << "Payroll information for " << LastName << ", " << FirstName <<
    ":" << endl << endl;
    cout << "Hours Worked: " << setw(24) << fixed << showpoint <<
    setprecision(2) << HoursWorked << endl;

    instead of

    cout << "Payroll information for ";
    cout << LastName << ", ";
    cout << FirstName << ":";
    cout << endl;
    cout << endl;
    cout << "Hours Worked: ";
    cout << setw(24);
    cout << fixed;
    cout << showpoint;
    cout << setprecision(2);
    cout << HoursWorked;
    cout << endl;

    There's a happy medium somewhere.

    Don't think that less code is better code all the time however.
    Sometimes splitting code up into small steps does aid understanding of
    that code.

    john
    John Harrison, Nov 10, 2005
    #7
  8. Guest

    wrote:
    > I turned this in for my programming fundamentals class for our second
    > exam. I am a c++ newb, this is my first class I've taken. I got a good
    > grade on this project I'm just wondering if there is a better and more
    > efficient way to write this program.


    Here's what I could come up with (yes, I'm bored :D).
    Compiles and runs, didn't verify the calculations.

    #include <iostream>
    #include <iomanip>
    #include <string>

    using namespace std;

    static const double state_tax_rate = 0.0825;

    static const unsigned federal_table_max = 4;
    static const double federal_rates_m[] = { 0.179, 0.168, 0.157, 0.146,
    0.125 };
    static const double federal_rates_s[] = { 0.151, 0.142, 0.133, 0.124,
    0.105 };

    int main()
    {
    string first_name, last_name;
    char married = 'U';
    double hours_worked = 0;
    double hourly_rate = 0;
    double gross = 0;
    double federal_tax = 0;
    double state_tax = 0;
    unsigned exemptions = 0;

    cout << "Enter your full name: ";
    cin >> first_name >> last_name;

    cout << "Hours worked: ";
    cin >> hours_worked;

    cout << "Hourly rate: ";
    cin >> hourly_rate;

    while( married != 'M'
    && married != 'S' )
    {
    cout << "Marrital status ('M' or 'S'): ";
    cin >> married;
    married = toupper( married );
    }

    cout << "Excemptions: ";
    cin >> exemptions;

    if ( exemptions > federal_table_max )
    exemptions = federal_table_max;
    gross = hours_worked * hourly_rate;
    state_tax = gross * state_tax_rate;
    federal_tax = gross * ( married == 'M' ?
    federal_rates_m[ exemptions ] : federal_rates_s[ exemptions ]
    );

    cout << endl;
    cout << fixed << showpoint << setprecision(2);

    cout << "Payroll information for ";
    cout << last_name << ", " << first_name << ":" << endl << endl;

    cout << "Hours worked: " << hours_worked << endl;
    cout << "Hourly wage: " << hourly_rate << endl;
    cout << "Marrital status: " << ( married == 'M' ? "Married" :
    "Single" ) << endl;
    cout << "Exemptions: " << exemptions << endl << endl;

    cout << "Gross pay: " << gross << endl;
    cout << "Federal tax: " << federal_tax << endl;
    cout << "State tax: " << state_tax << endl << endl;

    cout << "Net pay: " << (gross - federal_tax - state_tax )
    << endl;
    }

    Notice the difference? ;)

    Cheers,
    Andre
    , Nov 10, 2005
    #8
  9. Ian Guest

    wrote:
    > wrote:
    >
    >>I turned this in for my programming fundamentals class for our second
    >>exam. I am a c++ newb, this is my first class I've taken. I got a good
    >>grade on this project I'm just wondering if there is a better and more
    >>efficient way to write this program.

    >
    >


    > double gross = 0;
    > double federal_tax = 0;
    > double state_tax = 0;


    Remove these three;
    >


    > gross = hours_worked * hourly_rate;
    > state_tax = gross * state_tax_rate;
    > federal_tax = gross * ( married == 'M' ?
    > federal_rates_m[ exemptions ] : federal_rates_s[ exemptions ]
    > );

    make this:

    double gross = hours_worked * hourly_rate;
    double state_tax = gross * state_tax_rate;
    double federal_tax = gross * ( married == 'M' ?
    federal_rates_m[ exemptions ] : federal_rates_s[ exemptions ]);

    trivial, I know :)

    Ian
    Ian, Nov 10, 2005
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. octangle
    Replies:
    2
    Views:
    268
    Skarmander
    Jun 16, 2006
  2. Chris Richards
    Replies:
    8
    Views:
    137
    Peña, Botp
    Oct 31, 2007
  3. Stephan Wehner

    is there a better way to write { yield }

    Stephan Wehner, May 20, 2008, in forum: Ruby
    Replies:
    2
    Views:
    79
    Joel VanderWerf
    May 21, 2008
  4. Marc Bissonnette

    Is there a more efficient way to do this ?

    Marc Bissonnette, Jan 27, 2004, in forum: Perl Misc
    Replies:
    14
    Views:
    147
    Marc Bissonnette
    Jan 28, 2004
  5. Tim Chase
    Replies:
    0
    Views:
    85
    Tim Chase
    Dec 16, 2013
Loading...

Share This Page