Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have my program working. I just need to redo it a little bit, and it could use some improvements.

I got different % rates depending on what the income and status is.

#include <stdio.h>
#include <conio.h>
#include <stdlib.h>
#include <cstdio>

//functions called
float wages_loop();
float other_loop();
float interest_loop();
float dividends_loop();

int dependatnts_loop();

void check_status();
float get_total_income(float wage, float div, float intre, float other, int dep);

int single_total = 0;
int mj_total = 0;
int ms_total = 0;
int sh_total = 0;


//start main 
int main(void)
{
    char another[10];
    char buffer[80][90];

    float wages, other_income, interest, dividends, income_tax;
    int dependents;


    printf("Would you like to start: ");
    gets_s(another);

    if (another[0] != 'y' && another[0] != 'n')
    {
        while (another[0] != 'y' && another[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Would you like to start. (y or n)");
            gets_s(another);
        }
    }
    while (another[0] == 'y')
    {

        //add all info together.
        wages = wages_loop();
        other_income = other_loop();
        interest = interest_loop();
        dividends = dividends_loop();

        //enter dependats
        dependents = dependatnts_loop();

        //function to indicate the status and other things. 
        income_tax = get_total_income(wages, other_income, dividends, interest, dependents);

        if (income_tax < 0)
        {
            printf("\n\n\t\t Your income tax RETURN is: %.2f \n", income_tax);
        }
        else if (income_tax >= 0)
        {
            printf("\n\n\t\t Your income tax OWED is: %.2f \n", income_tax);
        }       

        printf("Would you like to do anoter: ");
        gets_s(another);

        if (another[0] != 'y' && another[0] != 'n')
        {
            while (another[0] != 'y' && another[0] != 'n')
            {
                printf("\n\n INCORRECT ANSWER. \n\n");
                printf("\n Would you like anoter. (y or n)");
                gets_s(another);
            }//end if
        }
    } //end loop

    printf("\n\n\t\t\t Number of Singles filleing: %i \n", single_total);
    printf("\n\n\t\t\t Number of Married Filing Jointly: %i \n", mj_total);
    printf("\n\n\t\t\t Number of Married Filing Separately: %i \n", ms_total);
    printf("\n\n\t\t\t Number of Single Head of Household filleing: %i \n", sh_total);

    system("pause");
    return 0;

}//end main


float wages_loop()
{
    char again[10];
    char buffer[80];
    float wages, total_wages = 0;

    printf("\n How much in Wages. ");
    gets_s(buffer);
    wages = atof(buffer);

    total_wages = wages + total_wages;

    printf("\n Do you have any more wages. (y or n)");
    gets_s(again);

    if (again[0] != 'y' && again[0] != 'n')
    {
        while (again[0] != 'y' && again[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Do you have any more wages. (y or n)");
            gets_s(again);
        }
    }

    while (again[0] == 'y')
    {
        printf("\n Enter Wages: ");
        gets_s(buffer);
        wages = atof(buffer);

        total_wages = wages + total_wages;

        printf("\n Do you have any more wages. ");
        gets_s(again);
    }

    return total_wages;
}//end wage_loop

float other_loop()
{
    char again[10];
    char buffer[80];
    float other_income, total_other_income = 0;

    printf("\n How much in other income. ");
    gets_s(buffer);
    other_income = atof(buffer);

    total_other_income = other_income + total_other_income;

    printf("\n Do you have any more other income. (y or n)");
    gets_s(again);

    if (again[0] != 'y' && again[0] != 'n')
    {
        while (again[0] != 'y' && again[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Do you have any more other income. (y or n)");
            gets_s(again);
        }
    }

    while (again[0] == 'y')
    {
        printf("\n Enter other income: ");
        gets_s(buffer);
        other_income = atof(buffer);

        total_other_income = other_income + total_other_income;

        printf("\n Do you have any more other income. ");
        gets_s(again);
    }

    return total_other_income;
}

float interest_loop()
{
    char again[10];
    char buffer[80];
    float interest, total_interest = 0;

    printf("\n How much in interest. ");
    gets_s(buffer);
    interest = atof(buffer);

    total_interest = interest + total_interest;

    printf("\n Do you have any more interest. (y or n)");
    gets_s(again);

    if (again[0] != 'y' && again[0] != 'n')
    {
        while (again[0] != 'y' && again[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Do you have any more interest. (y or n)");
            gets_s(again);
        }
    }

    while (again[0] == 'y')
    {
        printf("\n Enter interest: ");
        gets_s(buffer);
        interest = atof(buffer);

        total_interest = interest + total_interest;
        printf("\n Do you have any more interest. ");
        gets_s(again);
    }

    return total_interest;
}

float dividends_loop()
{
    char again[10];
    char buffer[80];
    float dividends, total_dividends = 0;

    printf("\n How much in dividends. ");
    gets_s(buffer);
    dividends = atof(buffer);

    total_dividends = dividends + total_dividends;

    printf("\n Do you have any more dividends. (y or n)");
    gets_s(again);

    if (again[0] != 'y' && again[0] != 'n')
    {
        while (again[0] != 'y' && again[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Do you have any more dividends. (y or n)");
            gets_s(again);
        }
    }

    while (again[0] == 'y')
    {
        printf("\n Enter dividends: ");
        gets_s(buffer);
        dividends = atof(buffer);

        total_dividends = dividends + total_dividends;
        printf("\n Do you have any more dividends. ");
        gets_s(again);
    }

    return total_dividends;
}//end dividends_loop


//dependants
int dependatnts_loop()
{
    char again[10];
    char buffer[80];
    int dependants, total_dependants = 0;

    printf("\n How much in dependants. ");
    gets_s(buffer);
    dependants = atof(buffer);

    total_dependants = dependants + total_dependants;

    printf("\n Do you have any more dependants. (y or n)");
    gets_s(again);

    if (again[0] != 'y' && again[0] != 'n')
    {
        while (again[0] != 'y' && again[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Do you have any more dependants. (y or n)");
            gets_s(again);
        }
    }

    while (again[0] == 'y')
    {
        printf("\n Enter dependants: ");
        gets_s(buffer);
        dependants = atof(buffer);

        total_dependants = dependants + total_dependants;
        printf("\n Do you have any more dependants. ");
        gets_s(again);
    }

    total_dependants = total_dependants * 2800;
    return total_dependants;
}



void check_status()
{
    char status[10];
    int check = 0;

    while (check != 1)
    {
        printf("What is your Status: ");
        gets_s(status);

        if (status[0] == 'S' && status[1] == 'H')
        {
            printf("\n\n CORRECT ANSWER. SH \n\n");
            single_total = single_total + 1;
            check = 1;
        }
        else if (status[0] == 'S' && status[1] == '\0')
        {
            printf("\n\n CORRECT ANSWER. S \n\n");
            single_total = single_total + 1;
            check = 1;
        }
        else if (status[0] == 'M' && status[1] == 'J')
        {
            printf("\n\n CORRECT ANSWER. MJ \n\n");
            mj_total = mj_total + 1;
            check = 1;
        }
        else if (status[0] == 'M' && status[1] == 'S')
        {
            printf("\n\n CORRECT ANSWER. MS \n\n");
            ms_total = ms_total + 1;
            check = 1;
        }
        else
        {
            printf("\n\n INCORRECT NSWER. noting \n\n");
        }
    }   
}


float get_total_income(float wage, float div, float intre, float other, int dep)
{
    char status[10];
    float income = 0, sum = 0, adjusted_income = 0;
    int check = 0;

    sum = wage + div + intre + other;
    income = sum - dep;

    while (check != 1)
    {
        printf("\n\nWhat is your Status: ");
        gets_s(status);

        if (status[0] == 'S' && status[1] == 'H')
        {
            sh_total = sh_total + 1;
            check = 1;
            if (income <= 6000)
            {
                adjusted_income = income * 0.0;
            }
            else if (income > 6000 && income <= 9000)
            {
                adjusted_income = income * .038;
            }
            else if (income > 9000 && income <= 15000)
            {
                adjusted_income = income * .074;
            }
            else if (income > 15000 && income <= 21000)
            {
                adjusted_income = income * .110;
            }
            else if (income > 21000 && income <= 25000)
            {
                adjusted_income = income * .138;
            }
            else if (income > 25000 && income <= 30000)
            {
                adjusted_income = income * .154;
            }
            else if (income > 30000)
            {
                adjusted_income = income * .35;
            }
            else
            {
                printf("\n\n INCORRECT ANSWER. CODE IS WRONG. \n\n");
            }
        }
        else if (status[0] == 'S' && status[1] == '\0')
        {
            single_total = single_total + 1;
            check = 1;
            if (income <= 6000)
            {
                adjusted_income = income * .028;
            }
            else if (income > 6000 && income <= 9000)
            {
                adjusted_income = income * .075;
            }
            else if (income > 9000 && income <= 15000)
            {
                adjusted_income = income * .096;
            }
            else if (income > 15000 && income <= 21000)
            {
                adjusted_income = income * .135;
            }
            else if (income > 21000 && income <= 25000)
            {
                adjusted_income = income * .155;
            }
            else if (income > 25000 && income <= 30000)
            {
                adjusted_income = income * .174;
            }
            else if (income > 30000)
            {
                adjusted_income = income * .35;
            }
            else
            {
                printf("\n\n INCORRECT ANSWER. CODE IS WRONG. \n\n");
            }
        }
        else if (status[0] == 'M' && status[1] == 'J')
        {
            mj_total = mj_total + 1;
            check = 1;
            if (income <= 6000)
            {
                adjusted_income = income * 0.0;
            }
            else if (income > 6000 && income <= 9000)
            {
                adjusted_income = income * .052;
            }
            else if (income > 9000 && income <= 15000)
            {
                adjusted_income = income * .083;
            }
            else if (income > 15000 && income <= 21000)
            {
                adjusted_income = income * .122;
            }
            else if (income > 21000 && income <= 25000)
            {
                adjusted_income = income * .146;
            }
            else if (income > 25000 && income <= 30000)
            {
                adjusted_income = income * .163;
            }
            else if (income > 30000)
            {
                adjusted_income = income * .35;
            }
            else
            {
                printf("\n\n INCORRECT ANSWER. CODE IS WRONG. \n\n");
            }
        }
        else if (status[0] == 'M' && status[1] == 'S')
        {
            ms_total = ms_total + 1;
            check = 1;
            if (income <= 6000)
            {
                adjusted_income = income * .023;
            }
            else if (income > 6000 && income <= 9000)
            {
                adjusted_income = income * .072;
            }
            else if (income > 9000 && income <= 15000)
            {
                adjusted_income = income * .089;
            }
            else if (income > 15000 && income <= 21000)
            {
                adjusted_income = income * .131;
            }
            else if (income > 21000 && income <= 25000)
            {
                adjusted_income = income * .152;
            }
            else if (income > 25000 && income <= 30000)
            {
                adjusted_income = income * .172;
            }
            else if (income > 30000)
            {
                adjusted_income = income * .35;
            }
            else
            {
                printf("\n\n INCORRECT ANSWER. CODE IS WRONG. \n\n");
            }
        }
        else
        {
            printf("\n\n INCORRECT STATUS. Enter (S, MJ, MS, or SH) \n\n");
        }
    }

    printf("\n\n\n\t YOUR WAGES: %.2f", wage);
    printf("\n\t YOUR OTHER INCOME: %.2f", other);
    printf("\n\t YOUR DIVIDENS: %.2f", div);
    printf("\n\t YOUR INTEREST: %.2f", intre);
    printf("\n\t YOUR INCOME AFTER DEPENDANTS: %.2f", income);

    return adjusted_income;
}
share|improve this question
1  
This is some sort of school project I guess? A couple of questions: 1. Have you learned about struct? 2. Have you learned about passing pointers to functions? (Example: void some_fun(int *store_value_here) { *store_value_here = 42; }) –  user13500 Feb 21 at 8:04
    
yes and no. I don't have to learn about struct yet. And far as pointers i couldn't wrap my head around the usefulness of it. –  user3255517 Feb 21 at 14:22
    
Thank you guys for the info. I am looking into this now. going to take some time. –  user3255517 Feb 21 at 18:34

3 Answers 3

up vote 20 down vote accepted

Things you did well:

  • You used the function gets_s, which is a C11 function. Not many people use this standard yet because it is newer. I was surprised to see it in your code.

  • Your organization of the prototype functions is good.

  • You initialize your variables as soon as you create them in some areas.

Things you could improve:

There is a lot that could be improved in this code, so I doubt I will be able to mention them all.

Preprocessor

  • You include both <stdio.h> and <cstdio>.

    #include <stdio.h>
    #include <conio.h>
    #include <stdlib.h>
    #include <cstdio>
    

    I couldn't get the code to compile as C code with the #include <cstdio> in there, so it should be removed.

User-experience

  • You ask the user if he is ready to start.

    printf("Would you like to start: ");
    gets_s(another);
    
    if (another[0] != 'y' && another[0] != 'n')
    {
        while (another[0] != 'y' && another[0] != 'n')
        {
            printf("\n\n INCORRECT ANSWER. \n\n");
            printf("\n Would you like to start. (y or n)");
            gets_s(another);
        }
    }
    

    The user initiated your program for a reason. Asking him if he would like start is useless, and can be frustrating to a user. To add onto the annoyance, you then tell the user that his input is "incorrect", if he doesn't input 'y', and you then loop the question. I would remove the whole thing.

Logic

  • Some of your logic can be simplified.

    if (status[0] == 'S' && status[1] == 'H')
    {
        printf("\n\n CORRECT ANSWER. SH \n\n");
        single_total = single_total + 1;
        check = 1;
    }
    else if (status[0] == 'S' && status[1] == '\0')
    {
        printf("\n\n CORRECT ANSWER. S \n\n");
        single_total = single_total + 1;
        check = 1;
    }
    

    Both times you are checking if status[0] == 'S', so use that as the "master" if test condition. Use the other tests as "children" tests.

    if (status[0] == 'S')
    {
        if (status[1] == 'H') puts("Correct answer: SH");
        if (status[1] == '\0') puts("Correct answer: S");
        single_total = single_total + 1;
        check = 1;
    }
    

    Pull out the code that the original test conditions had in common to the "master" condition, and you have a refined test condition statement!

  • You sometimes have logic that will print out to the console that the logic in the code is wrong.

    else
    {
        printf("\n\n INCORRECT ANSWER. CODE IS WRONG. \n\n");
    }
    

    I would use assert() instead. If this expression evaluates to 0, this causes an assertion failure that terminates the program. Also, assertions are the right mechanism to use, since those positions in the code would only be reachable due to programmer error, not due to unanticipated runtime conditions.

    else
    {
        assert(income > 30000);
        adjusted_income = income * .35;
    }
    

Variables

  • You have a lot of "magic numbers" in your code.

    if (income <= 6000)
    {
        adjusted_income = income * 0.0;
    }
    else if (income > 6000 && income <= 9000)
    {
        adjusted_income = income * .038;
    }
    else if (income > 9000 && income <= 15000)
    {
        adjusted_income = income * .074;
    }
    else if (income > 15000 && income <= 21000)
    {
        adjusted_income = income * .110;
    }
    else if (income > 21000 && income <= 25000)
    {
        adjusted_income = income * .138;
    }
    else if (income > 25000 && income <= 30000)
    {
        adjusted_income = income * .154;
    }
    else if (income > 30000)
    {
        adjusted_income = income * .35;
    }
    

    You should extract all of those numbers to variables in case you have to change them later. Then you only have to change one number in one place, instead of changing the number in multiple different places. What if you missed a place?

  • Your variable char buffer[80][90] is unused and should be removed.

  • It's generally good practice to initialize all of your non-static variables when possible.

  • Don't use global variables.

    int single_total = 0;
    int mj_total = 0;
    int ms_total = 0;
    int sh_total = 0;
    

    The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.

    If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.

Syntax

  • You use too much space when printing to the console. Also, use puts() instead of printf() in some cases where you are not actually formatting the string but just printing it with a newline character at the end.

  • mj_total = mj_total + 1 can be simplified to mj_total += 1.

Input

  • Your code can't handle the input of a string properly.

    How much in other income. ten
    

    It should tell the user that it is unacceptable input and ask for re-entry of the data.

  • Your program can't handle the input of a malformed string properly.

    How much in other income. 107t
    

    It should tell the user that it is unacceptable input and ask for re-entry of the data.

  • Your program can't handle the input of a \n (new-line) character (pressing enter).

    How much in other income. <enter>
    

    It should tell the user that it is unacceptable input and ask for re-entry of the data.

  • Your program is very unforgiving when asking the status of the user.

    printf("\n\nWhat is your Status: ");
    gets_s(status);
    

    If you input a lower-case character, it won't be accepted. Use the toupper() function on your input character to fix this. You have this same issue when asking the user if he has more wages, dividends, etc.

    printf("\n Would you like anoter. (y or n)");
    gets(another);
    

    Here you might want to use the tolower() function.

share|improve this answer
3  
The only thing I would suggest for this post is adding {} to the single line if statements. It's a coding-style issue, but the OP is also just starting programming. I've seen all sorts of errors/bugs pop up due to unbracketed ifs, fors, whiles, etc, so imo it's just better to explicitly use {}, even for one line statements where it doesn't make a semantic difference. –  Ryan Feb 21 at 15:06
    
your logic is not idempotent in the case where status = S{anything but H or \0} –  njzk2 Feb 21 at 18:55
    
@njzk2 That was only a snippet of the longer logic block. –  syb0rg Feb 21 at 19:10
    
@sybo0rg still, in the original code, SH and S\0 are considered good answers, in your case, anything that starts with an S goes into that block, getting single_total to be incremented. –  njzk2 Feb 21 at 20:20
    
@njzk2 If the user enters S into the program in any form, he is indicating that he is single in some way, so the single_total should be incremented if the input starts with an 'S' in my opinion. –  syb0rg Feb 21 at 22:35

Language Usage Problems

You call gets_s() with only one parameter. It expects two parameters — the second parameter is supposed to be the size of the buffer. I would be surprised that this compiled at all.

You tagged this question as , and it is indeed mostly C. However, you #include <cstdio>, which is a C++ header file, instead of <stdio.h>, which is what you should use in C. (<cstdio> provides declarations for C-style I/O functions to C++ programs, for compatibility with legacy C code.)

If you #include <stdio.h>, I don't believe you need <conio.h>, which is Windows-specific.

system("pause") works on Windows, but is not portable. getchar() would be a portable alternative.

The check_status() function is defined but never used. If you compile with warnings enabled, your compiler should tell you that char buffer[80][90] in main() is an unused variable.

Logic Bug

Compare the signature of this function

float get_total_income(float wage, float div, float intre, float other, int dep)
{
    …
}

against the way you call it:

income_tax = get_total_income(wages, other_income, dividends, interest, dependents);

Notice the discrepancy?

Code Organization

You input routines include nearly identical code, repeated four times. Instead, you should be able to define a single function to be used four times, like this:

wages = prompt_amount_in_category("wages");
other_income = prompt_amount_in_category("other income");
interest = prompt_amount_in_category("interest");
dividends = prompt_amount_in_category("dividends");

Your functions could be named better. For example, dependents_loop() would be better named prompt_number_of_dependents(). It reads better as English and is more descriptive. Also, the caller is more interested in the purpose of the function; the fact that there happens to be a loop inside the function is an implementation detail that doesn't deserve to be part of the function's name.

get_total_income() is a misnomer — it actually does much more than that. After calculating the total income, it also asks the user for the marital/filing status, increments a global counter based on that answer, calculates the tax, and displays a report. A function should do one thing only, and its name should reflect its purpose.

enum filing_status {
    SINGLE,
    MARRIED_FILING_JOINTLY,
    MARRIED_FILING_SEPARATELY,
    SINGLE_HEAD_OF_HOUSEHOLD
};
#define FILING_STATUS_MAX (SINGLE_HEAD_OF_HOUSEHOLD + 1)

float get_total_income(float wage, float div, float intre, float other, float dep)
{
    return wage + div + intre + other + dep;
}

enum filing_status prompt_filing_status()
{
    char status[10];
    do {
        printf("\n\nWhat is your Status: ");
        gets_s(status, sizeof(status));
        switch (status[0]) {
          case 'S': case 's':
            switch (status[1]) {
              case '\0':
                return SINGLE;
              case 'H': case 'h':
                return SINGLE_HEAD_OF_HOUSEHOLD;
            }
            break;
          case 'M': case 'm':
            switch (status[1]) {
              case 'J': case 'j':
                return MARRIED_FILING_JOINTLY;
              case 'S': case 's':
                return MARRIED_FILING_SEPARATELY;
            }
            break;
        }
        printf("\n\n INCORRECT STATUS. Enter (S, MJ, MS, or SH) \n\n");
    } while (1);
}

float calculate_tax(float income, enum filing_status status)
{
    …
}

int main() {
    int return_counts[FILING_STATUS_MAX] = { 0 };
    …

    wages = prompt_amount_in_category("wages");
    other_income = prompt_amount_in_category("other income");
    interest = prompt_amount_in_category("interest");
    dividends = prompt_amount_in_category("dividends");
    dependants = -2000 * prompt_number_of("dependants");

    //function to indicate the status and other things. 
    enum filing_status status = prompt_filing_status();
    return_counts[status]++;

    total_income = get_total_income(wages, dividends, interest, other_income, dependents);
    income_tax = calculate_tax(total_income, status);


    printf("\n\n\n\t YOUR WAGES: %.2f", wages);
    printf("\n\t YOUR OTHER INCOME: %.2f", other_income);
    printf("\n\t YOUR DIVIDENS: %.2f", dividends);
    printf("\n\t YOUR INTEREST: %.2f", interest);
    printf("\n\t YOUR INCOME AFTER DEPENDANTS: %.2f", total_income);

    …
}

Accountancy Errors

According to your program, a single filer with 9000.00 of income owes 468.00 in income tax. A single filer with 9000.01 in income owes 747.00 in income tax. That's the most regretful cent that anyone has ever earned! Tax brackets don't work like that — each marginal tax rate only applies to the part of the income that falls within that tax bracket, not to the entire income.

I know that this is just a toy program, but such an error makes it awkward for me to suggest a way to reduce the amount of code to compute the tax. I could either propose a buggy solution that is compatible with your original, or a plausible solution that is hard to compare against the original code. Therefore, I'll just end the review here without improving calculate_tax().

share|improve this answer
    
I should point out that I was the one who removed the C++ tag. Only that one header was from C++, and wasn't enough for the tag. –  Jamal Feb 21 at 4:39
    
@Jamal Thanks for the clarification. The point remains, though: the original question claimed to be polyglot c/c++, and it wasn't c. –  200_success Feb 21 at 4:43
    
Even if it were to be reviewed as C++ code as well, it would need even more changes. This is from an assignment, so perhaps it was meant to be C++, but without the STL. Perhaps I was wrong to remove the tag, or we just need more clarification from the OP. –  Jamal Feb 21 at 4:47
    
Like your "prompt" naming. Should have used that. The get_income_group becomes a miss compared to the other get_ functions. –  user13500 Feb 21 at 15:47

New to this site, so hope this is in correct format. I did a rewrite of your code. I also notice others has entered some of the same conclusions, so some redundancy.

I assume this is a school project and as such I will omit some things that I otherwise would have used. Among other things struct's.

Ended up with a rather big rewrite. It is not certain you will find it useful, but hopefully you can draw something from it. I almost feel a bit rude here, but.

As this text likely is easier to read and understand with the complete source for the rewrite in one collective file i have pasted it here.

OK. Lets start.


Firstly I will mention that I use the fprintf() version. This is from habit as if I later find the need to output to something else then stdout it is easy to change. I also have some coding style when it comes to printf() that I have found to be working well.

I have also not used gets_s() as it simply is not available on my system.

User input

When we look at your code one of the things that is most clear is the redundancy in user input functions. The functions:

float wages_loop();
float other_loop();
float interest_loop();
float dividends_loop();

can be merged into one function. If we notice we repeat our selves, we should take a good long look at what we are doing and how we can re factor the code.

Secondly it is a good rule to have as number one:

  1. A function should: Do one thing, and do it well.

A second thing that we use when appropriate is helper functions. This both helps us splitting up the code into manageable parts, as well as make the code much less redundant.

Lets start with confirmations. We have repeated requests for confirmation where answer is y for yes and n for no. Using the power of printf() we can then make a general confirmation function.

Secondly we have input by float and int. Dividing this into functions we can end up with something like this. (Hopefully they should be rather self-explaining).

We should have had both int and float versions + some error checking, but this should be an OK starter. For int input I cast the float.

static int confirm(char *title)
{
        char buf[10];

        fprintf(stdout, "%s (y or n) ", title);
        while (1) {
                fgets(buf, sizeof(buf), stdin);
                switch (*buf) {
                case 'y': case 'Y':
                        return 1;
                case 'n': case 'N':
                        return 0;
                }
                fprintf(stderr, "Please answer with y for Yes or n for No: ");
        }
}

static float get_float(char *title) {
        char buf[80];

        fputs(title, stdout);
        fgets(buf, sizeof(buf), stdin);
        return atof(buf);
}

static float get_info_float(char *type)
{
        float amount, total = 0;

        fprintf(stdout, "\n -- %s --\n", type);
        do {
                amount = get_float("\n Enter amount: ");
                total += amount;
                fprintf(stdout, "\n Do you have any more %s? ", type);
        } while(confirm(""));

        return total;
}

In the get_info_float() function we could also have informed the user to enter 0 to abort request for values. That would perhaps be more user-friendly then a "y/n request - one more?" for each input.

Result printing

The next thing we can take a look at is printing information. I like to put these into separate functions. Usually there would have been the use of struct instead of loads of parameters, but lets go for it this time.

The two main print functions can be defined like this:

static void print_result(
                float wages,
                float income_other,
                float interests,
                float dividends,
                float income_dep,
                float income_adj
) {
        int hdr_w = 29;

        fprintf(stdout,
                "\n\n"
                "-----------------------------------------------\n"
                "  %-*s: %.2f\n"
                "  %-*s: %.2f\n"
                "  %-*s: %.2f\n"
                "  %-*s: %.2f\n"
                "  %-*s: %.2f\n"
                "\n"
                "  %-*s: %.2f\n"
                "-----------------------------------------------\n"
                "\n"
                ,
                hdr_w, "YOUR WAGES", wages,
                hdr_w, "YOUR OTHER INCOME", income_other,
                hdr_w, "YOUR INTEREST", interests,
                hdr_w, "YOUR DIVIDEND", dividends,
                hdr_w, "YOUR INCOME AFTER DEPENDENTS", income_dep,
                hdr_w,
                (
                 income_adj < 0 ?
                 "Your income tax RETURN is" :
                 "Your income tax OWED is"
                ),
                income_adj
        );
}
static void print_filing_stats(
              int files_s,
              int files_mj,
              int files_ms,
              int files_sh
) {
        int hdr_w = 44;

        fprintf(stdout,
                "%-*s: %i\n"
                "%-*s: %i\n"
                "%-*s: %i\n"
                "%-*s: %i\n"
                ,
                hdr_w, "Number of Singles Filing", files_s,
                hdr_w, "Number of Married Filing Jointly", files_mj,
                hdr_w, "Number of Married Filing Separately",files_ms,
                hdr_w, "Number of Single Head of Household Filing", files_sh
        );
}

Note the use of a variable to adjust the labels/headers namely by hdr_w. This makes it very easy to make nice prints as well as adjusting the width if we find ourselves expanding or removing print data.

I am also always very strict in the formatting as it makes it that much easier to maintain and keep track of.

If you are not used to the ternary operator, income_adj < 0 ? "one thing" : "other thing" you can separate that out to if's, but I find it rather powerful in combination with printf.

Income groups

Your biggest function is the income testing and re-calculation. As you are familiar with arrays we can simplify this a lot by using them. We declare one adjustment array for each filing type, and one holding the different income groups.

These could have been local to functions, but decided instead to have them in global scope and declare them as const – as such they should not be modified. We could further set them static if you are familiar with that.

const float adjust_SH[]  = { 0.000, 0.038, 0.074, 0.110, 0.138, 0.154, 0.350 };
const float adjust_S[]   = { 0.028, 0.075, 0.096, 0.135, 0.155, 0.174, 0.350 };
const float adjust_MJ[]  = { 0.000, 0.052, 0.083, 0.122, 0.146, 0.163, 0.350 };
const float adjust_MS[]  = { 0.023, 0.072, 0.089, 0.131, 0.152, 0.172, 0.350 };

const int income_group[] = { 6000, 9000, 15000, 21000, 25000, 30000 };

#define STATUS_S        1
#define STATUS_MJ       2
#define STATUS_MS       3
#define STATUS_SH       4

#define STATUS_MIN      1
#define STATUS_MAX      4

#define INCOME_GR_LEN   6

Instead of checking the income by if's we can now make a short function doing the same by looping. Also note in your original code that you do not need to check for both bigger and lower in each if statement. You can simply start at bottom and say

if (income <= 6000)      ... do something
else if (income <= 9000) ... do something etc.

As by else if you will not have to check that it is greater then. Anyhow, by array we can also do this:

/*
 * Return index of adjust_XX array for income group.
 */
static int get_income_group(float income)
{
        int i;

        for (i = 0; i < INCOME_GR_LEN; ++i) {
                if (income <= income_group[i])
                        return i;
        }
        return i; /* As the adjust arrays are one item longer then the
                   * income_group array, this will give us last index. */
}

Filing / Status types

Instead of using the a bit cumbersome acronyms and check by char we print a short menu and ask for numeric input. As we already have the get_float() helper function this also makes it for a short and concise function:

static int get_status() {
        int status = 0;

        fputs(  "\n"
                " Status types:\n"
                "   1. Single Fileing\n"
                "   2. Married Filing Jointly\n"
                "   3. Married Filing Separately\n"
                "   4. Single Head of Household Fileing\n"
                "\n"
                ,
                stdout
        );

        do {
                status = (int)get_float("Enter Status type number: ");
        } while (status < STATUS_MIN && status > STATUS_MAX);

        return status;
}

Adjusted income

By this the final adjust_income() function can be written like:

static float get_income_adjusted(
                float inc_dep,
                int status
) {
        float adjusted_income = 0;
        int inc_group;

        /* If we are to follow the same requirement as in the
         * "if" chain in the original function, we check that
         * income minus dep is above 0. */
        assert(inc_dep > 0);
        if (inc_dep < 0) {
                fprintf(stderr, "FAIL!\n");
                return 0.0;
        }

        inc_group    = get_income_group(inc_dep);

        /* Switch status / filing type and multiply by array 
         * value given index by income group */
        switch (status) {
        case STATUS_SH:
                adjusted_income = inc_dep * adjust_SH[inc_group];
                break;
        case STATUS_S:
                adjusted_income = inc_dep * adjust_S[inc_group];
                break;
        case STATUS_MJ:
                adjusted_income = inc_dep * adjust_MJ[inc_group];
                break;
        case STATUS_MS:
                adjusted_income = inc_dep * adjust_MS[inc_group];
                break;
        }

        return adjusted_income;
}

Back to main()

To stitch it together we can go back to main(). We already have discussed the print functions, and as such there is one tricky thing left, and that is the global counters for type of filing. We can solve this by yet another array, else we would likely have passed a struct by address, but lets do it this way:

int main(void)
{
        int loop,
            filing,
            filings[] = {0, 0, 0, 0};

        loop = 1; // = confirm("Should we start? ");

        while (loop) {

                filing = process_filing();
                filings[filing] += 1;

                loop = confirm("Would you like to file another? ");
        }

        print_filing_stats(
                filings[0],
                filings[1],
                filings[2],
                filings[3]
        );

        fprintf(stderr, "\nPress enter to quit. ");
        getchar();

        return 0;
}

We use the filings[] = {0, 0, 0, 0}; array to count type of filing returned by process filing. Look at the signature for print_filing_stats() to get how it is stitched together.

Final count up

Now the final piece of the puzzle is the process_filing() function. The main motor. By now it is simply a row of instructions and function calls that should be easy to follow.

#define DEP_FACTOR       2800
int process_filing()
{
       float wages,
              income_other,
              interests,
              dividends,
              income_dep,
              income_adj,
              sum;
        int dependents,
            status;

        wages           = get_info_float("Wages");
        income_other    = get_info_float("Other income");
        interests       = get_info_float("Interest");
        dividends       = get_info_float("Dividends");

        dependents      = DEP_FACTOR * (int)get_info_float("Dependents");

        sum             = wages + income_other + interests + dividends;
        income_dep      = sum - dependents;

        status = get_status();

        income_adj = get_income_adjusted(
                        income_dep,
                        status
        );

        print_result(
                wages,
                income_other,
                interests,
                dividends,
                income_dep,
                income_adj
        );

        return status;
}

Final note

I have mostly looked at the structuring of the code, and not validated a correct result. Hope it or some of it can be useful anyhow.

share|improve this answer
    
Very nicely done. You did some things I don't agree with, such as using multiple #defines instead of an enum, but overall you give some very good advice. –  syb0rg Feb 21 at 14:24
    
@syb0rg: Thanks :) and yes, I was actually considering enum, but was very unsure on what level to put it. struct and pointers was another consideration. At the end I even considered posting it as a new "review this please" post, but guess an answer here was better. –  user13500 Feb 21 at 14:44

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.