6
\$\begingroup\$

I'm trying to optimize this code by:

  1. condensing repetitive statements
  2. Improving the code/make it less confusing
  3. Branch out to include other filing status such as "married" "joint" filing status
  4. branch out to all 50 states

Any suggestions how to do any of these things would be awesome.

/** This program calculates Single-filing 2015 Federal Tax Schedule and Georgia State income Tax For GA residents making >= $7,000 */

import java.util.Scanner;

public class fedIncomeTax
{
public static void main(String[] args)
{
Scanner in = new Scanner(System.in);
System.out.print("Enter income over $7,000: "); // this is because there are different tax rates for GA residents who earn below that mark

if (in.hasNextDouble()) // Checks whether the next input is a real number && if false prints error message
  {

 double begIncome = in.nextDouble();

 // Georgia State tax for incomes >= $7,000
 final double GA_TAX_1 = 0.06;
 final double GA_TAX_2 = 230;


 // fed tax rates
 final double FED_TAX_10 = 0.10; // $1 - 9075
 final double FED_TAX_15 = 0.15; // $9076 - 36900
 final double FED_TAX_25 = 0.25; // $36901 - 89350
 final double FED_TAX_28 = 0.28; // $89351 - 186350
 final double FED_TAX_33 = 0.33; // $186,351 - 405,100
 final double FED_TAX_35 = 0.35; // $405,101 - $406,750
 final double FED_TAX_39 = 0.396; // > $406,750

 // income brackets
 final double BRACKET_10 = 9075;
 final double BRACKET_15 = 36900;
 final double BRACKET_25 = 89350;
 final double BRACKET_28 = 186350;
 final double BRACKET_33 = 405100;
 final double BRACKET_35 = 406750;
 final double BRACKET_39 = 406751;

 // max amount per bracket
 final double MAX_TAX_15 = 907.50;
 final double MAX_TAX_25 = 5081.25;
 final double MAX_TAX_28 = 18193.75;
 final double MAX_TAX_33 = 45353.75;
 final double MAX_TAX_35 = 117541.25;
 final double MAX_TAX_39 = 118118.75;


 //double begIncome = in.nextDouble();
 if (begIncome < 7000)
 {
    System.out.println("Error: Please enter an income figure of $7000 or more.");
 }

 // Now we know that the user entered a real number

 else
 {  

    // Compute taxes by income range


    if (begIncome <= BRACKET_10)                                               // $1 - 9075
    {
       double govTax = begIncome * (FED_TAX_10);
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }

    else if (begIncome > BRACKET_10 && begIncome <= BRACKET_15)            // $9076 - 36900
    {
       double govTax = (begIncome - BRACKET_10) * FED_TAX_15 + MAX_TAX_15;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }

    else if (begIncome > BRACKET_15 && begIncome <= BRACKET_25)            // $36901 - 89350
    {
       double govTax = (begIncome - BRACKET_15) * FED_TAX_25 + MAX_TAX_25;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }

    else if (begIncome > BRACKET_25 && begIncome <= BRACKET_28)            // $89351 - 186350
    {
       double govTax = (begIncome - BRACKET_25) * FED_TAX_28 + MAX_TAX_28;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }

    else if (begIncome > BRACKET_28 && begIncome <= BRACKET_33)            // $186,351 - 405,100
    {
       double govTax = (begIncome - BRACKET_28) * FED_TAX_33 + MAX_TAX_33;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }

    else if (begIncome > BRACKET_33 && begIncome <= BRACKET_35)            // $405,101 - $406,750
    {
       double govTax = (begIncome - BRACKET_33) * FED_TAX_35 + MAX_TAX_35;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }
    else if (begIncome > BRACKET_35)                                         // > $406,750
    {
       double govTax = (begIncome - BRACKET_35) * FED_TAX_39 + MAX_TAX_39;
       double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
       double totalTax = govTax + localTax;
       double endIncome = begIncome - totalTax;
       System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
       System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    }    
 }

 }               
 else
 {
  System.out.println("Error: Please input a numerical value greater than 0.");
 }



}
}  
\$\endgroup\$
  • 3
    \$\begingroup\$ Just a tip you should look for an online API or look up something that is kept up to date, otherwise you have to manually program all the tax rates/brackets every year if they change. \$\endgroup\$ – JBaczuk Feb 5 '15 at 14:13
  • \$\begingroup\$ @JBaczuk - thats true, I definitely don't want to update it every single year. \$\endgroup\$ – CSCI_uga Feb 5 '15 at 14:22
  • \$\begingroup\$ Your SO question was migrated here, and I merged them. Feel free to adjust your 'accept' mark if you feel inclined. \$\endgroup\$ – rolfl Feb 5 '15 at 16:32
  • \$\begingroup\$ If you are using this for real financial calculations, be very cautious about the use of float or double to store values. Rounding errors can become an issue. Fixed-precision or something like BigDecimal is a better way to do it. \$\endgroup\$ – tpg2114 Feb 5 '15 at 21:26
6
\$\begingroup\$

That huge if/else statement you have is terrible.

First, you print out the values using the same print statement at the end of each if, just move that to the end:

else if (begIncome > BRACKET_35)                                         // > $406,750
{
   double govTax = (begIncome - BRACKET_35) * FED_TAX_39 + MAX_TAX_39;
   double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
   double totalTax = govTax + localTax;
   double endIncome = begIncome - totalTax;
   System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
   System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
}

Becomes:

else if (begIncome > BRACKET_35)                                         // > $406,750
{
   double govTax = (begIncome - BRACKET_35) * FED_TAX_39 + MAX_TAX_39;
   double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
   double totalTax = govTax + localTax;
   double endIncome = begIncome - totalTax;
}  

System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
System.out.printf ("Your ending income is:  $ %8.2f", endIncome);

Then, you have the same code run with different values in each of the if statements. Why not separate that into a method, and pass the values needed to calculate the required values? If you do this, you would be able to change your if into something like this:

else if (begIncome > BRACKET_33 && begIncome <= BRACKET_35)            // $405,101 - $406,750
{
   CalcTax(begIncome, BRACKET_33);
}
else if (begIncome > BRACKET_35)                                         // > $406,750
{
   CalcTax(begIncome, BRACKET_35);
}

If you do this, you may want to make your final static variables global so you don't have to pass them to the method - both methods will just be able to access them. However, as a general rule, try to keep the scope of variables as tight as possible.

Also, instead of having different variables for the different tax values, you could create an array of tax brackets with all the values:

final static int[] TaxBrackets = { 9075, 36900, /* other values... */ };
\$\endgroup\$
  • \$\begingroup\$ it's my first yr taking programming classes so a lot of your suggestions are really helpful. That being said, when I move the print statements out of the else if, I get a system error "cannot find variable".. how do I fix this? \$\endgroup\$ – CSCI_uga Feb 5 '15 at 20:31
  • \$\begingroup\$ You need to move the variable out of main so it is just in the class. If you do this, it will be accessible by all the methods. \$\endgroup\$ – Hosch250 Feb 5 '15 at 20:32
  • \$\begingroup\$ @CSCI_uga: The variable scopes are contained within the conditionals, so anything outside will not recognize them. \$\endgroup\$ – Jamal Feb 5 '15 at 20:36
5
\$\begingroup\$

Constants

  • Constants denoted with final should also be static.

  • They should either be placed above main() or in another class. When programming in Java, it is important to utilize multiple classes whenever possible. Putting everything into main(), especially with more complex programs such as this, will prove to be inexcusable.

  • The income brackets constants can just be ints. Also, as @barq has mentioned, the other constants should just be floats since precision isn't recommended for monetary values.

Misc.

  • Try to keep indentation and whitespace consistent. It's all over the place here, which just makes it harder for others to read your code. One place to start is Google Java Style.

  • The initial user input looks confusing. First you prompt the user for a minimum value, check a different condition, define some constants, then finally check this value. You should instead do such checking at the start, and either repeat the repeat the prompt until a valid value is entered, or just terminate the program. If the latter, then you may also need to throw an exception.

    The hard-coded 7000 is also a "magic number" and should be given a constant. Always try to do this with numerical values whose purposes aren't obvious. This will also make it easier to maintain this value for possible changes.

  • When printing out an error, send it to System.err instead of System.out.

\$\endgroup\$
2
\$\begingroup\$
  1. Make use of List or an array in your case to store GA/FED tax(separate one for each tax)
  2. The only thing that is changing is calculation of govTax. So define double govTax outside of if/else block and rest of the code i.e. repeated code below:

    double localTax = (begIncome * GA_TAX_1) + GA_TAX_2;
    double totalTax = govTax + localTax;
    double endIncome = begIncome - totalTax;
    System.out.printf("Your tax rate is:       $ %8.2f\n", totalTax);
    System.out.printf ("Your ending income is:  $ %8.2f", endIncome);
    

just before you end your else condition like:

else {
     if ..{
     } else if (..){
     }
     localTax...//repeated statement starts here
     ..
     prints..
}
\$\endgroup\$
  • \$\begingroup\$ Yeah I figured there had to be a better way to condense this since it is a pretty simple program. I'll definitely look into lists/arrays. Thanks \$\endgroup\$ – CSCI_uga Feb 5 '15 at 14:24
2
\$\begingroup\$

Style: By convention, class names should be capitalized, i.e. FedIncomeTax.

Formatting: Indent after opening { and unindent after closing }.

Use float instead of double for your constants. You are not making use of the extra precision.

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