Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

My task is ask this from the user:

Enter the number corresponding to the desired pay rate or action:

1) $8.75/hr           2) $9.33/hr

3) $10.00/hr          4) $11.20/hr

5) quit

If choices 1 through 4 are selected, the program should request the hours worked. The program should recycle until 5 is entered. If something other than choices 1 through 5 is entered, the program should remind the user what the proper choices are and then recycle. And i have to use switch

My code:

#include <stdio.h>
#define RATE1 8.75
#define RATE2 9.33
#define RATE3 10.00
#define RATE4 11.20

int main()

{
    int rateSelected;
    int weeklyHours;
    double totalPayed, rateToCalculate;

    printf("Enter the number corresponding to the desired pay rate or action:\n");
    printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
    printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
    printf("5) Quit\n");


    while ((scanf("%d", &rateSelected)) != EOF && rateSelected != 5)
    {

        if (rateSelected > 4)
        {
            printf("please enter a valid number:\n");
            continue;
        }


            switch (rateSelected)
        {
            case 1:
                rateToCalculate = RATE1;
                break;
            case 2:
                rateToCalculate = RATE2;
                break;
            case 3:
                rateToCalculate = RATE3;
                break;
            case 4:
                rateToCalculate = RATE4;
                break;

        };


        printf("please enter you weekly hours:\n");
        scanf("%d", &weeklyHours);
        totalPayed = weeklyHours * rateToCalculate;
        printf("your paychack for this week is: %.2lf\n\n",totalPayed);

        printf("Enter the number corresponding to the desired pay rate or action:\n");
        printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
        printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
        printf("5) Quit\n");

    }

    return 0;
}

How bad is it? And one thing didn't work for me is if a user put a letter its not cycling..(any suggestion for that too?)

Thank you,

share|improve this question

2 Answers

up vote 2 down vote accepted

It's not bad. A couple of issues regarding validating input, which you know about, but a few 'style' things, some of which you may not know about yet.

In addition to Ryan's answer, a couple more suggestions:

  • Only declare one variable per line. Otherwise it becomes harder to see where it comes from.

    double totalPayed;
double rateToCalculate;

  • In general, try to declare variables in the inner-most block in which they're used. In this case, all but rateSelected should be declared inside the while loop. It doesn't matter for this, but in more complex programs restricting where the variables can be used will become more important.

  • Don't have 'magic numbers', in this case the options themselves. I would suggest setting up an enum (if you know about them?, or a series of #defines), something like:

    enum { OPTION_BEGIN = 1, OPTION_RATE_ENTRY = OPTION_BEGIN, OPTION_RATE_MID, OPTION_RATE_SENIOR, OPTION_RATE_MANAGER, OPTION_QUIT, OPTION_END };

    Use these in getRate instead of the appropriate numbers.

  • Separate out the question into a separate function, it's an identifiable single task. And it becomes easier to change if it's one place. I've added another function here for readability:

    int is_valid_option( int option )
{
    return ( option >= OPTION_BEGIN ) && ( option < OPTION_END );
}

int ask_for_pay_rate( void )
{
    int option = OPTION_END;
    while ( option == OPTION_END ) 
    {
        int answer;

        printf("Enter the number corresponding to the desired pay rate or action:\n");
        printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
        printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
        printf("5) Quit\n");

        /* scanf returns the number of successful inputs, ie 1 number.
           Test for this, rather than eof */
        if ( ( scanf( "%d", &answer ) == 1 ) && is_valid_option( answer ) )
        {
            option = answer;
        }
        else
        {
            printf("please enter a valid number:\n");
        }
    }
    return option;
}

    Note by the way, that I've set up a condition at the start of the while loop which forces it to execute at least once. Doing it the way you did means that you have to duplicate code, allowing the possibility that it'll be subtly different.

  • You may want to validate the value for weeklyHours, should be a number >= 0

share|improve this answer
1  
thank allot for that detailed comment! it helped allot :) @Glenn Rogers –  Nir Jan 30 at 11:00
up vote 1 down vote
  1. Create better names for the rate levels, maybe you could use positions or titles (e.g. entry level rate, mid level rate, etc.).
  2. Consistent use of whitespace would help readability
  3. Pull the switch statement out into a different method

like below: 

int getRate(RateEnum selection)
{
    switch (selection)
    {
        case 1:
            return EntryLevelRate;
            break;
        case 2:
            return MidLevelRate;
            break;
        case 3:
            return SeniorLevelRate;
            break;
        case 4:
            return ManagerLevelRate;
            break;
    };
}

Then call it like this: rateToCalculate = getRate(rateSelected);

If you are trying to prompt the user again, you should make use of a while loop.

share|improve this answer
 
Thank allot :) @RyanGates –  Nir Jan 30 at 11:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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