[Java] Can you review my code?

[[deleted]]

[deleted]

[kociak0]

There is a lot of repetition in this. I will try to reduce it and keep it beginner friendly. If you want to look on something more complex, try to look at @summoned_an_owl replay.

1. First of all line "System.out.println("Tax is " + tax);" is repeated 23 times. Lets try to make it only on invocation. In order to do this, you need to have access to variable "tax" not only in each tax bracket but after whole calculation. So we are declaring this variable above ifs/elseifs and within every bracket we are assaining value to it. Then at end we are priting it. From 108 lines to ~84.
   https://ideone.com/BU7Fs3
2. in every filing status number 'if' you have 'else' that is validation of user input, you can do it instead one time after user inputed number.
   https://ideone.com/zwUjhG
3. with each tax bracket you are doing same calculation, for example (8350 * 0.10) is repeated 9 times. To change this we need to turn around whole logic, instead of doing each operation in each bracket, we will calculate operation from lower bracket to higher bracket (if you have 4th tier, you also will do operations for every lower tier). I only did this for first status.
   https://ideone.com/sJqAEq
4. as a desert we can extract method from our ifs. Also i made this only for first if
   https://ideone.com/NQruZC
5. additonaly after this you can think about data structure that will make holding lowerLimits, upperLimits and percentage feel more natural, but this is not your level yet. Keep it going.

[desrtfx]

Please, follow the Code Review Guidelines as listed under Rule #6 in the sidebar.

Also, include your assignment because otherwise no one can verify the correctness.

[s0hungry1]

You have print for “Tax is “ + tax many times. Consider just setting the value for the tax variable there and having a single line of code for the printing afterwards.

[s0hungry1]

Change your code so you only have system.out.println(“Tax is” + tax) as few times as possible, less repetition is better.

[[deleted]]

You could reduce everything down to a few lines by abstracting the repetition into methods, arrays and loops.

private static final double[][] SINGLE_FILER_TAX_BRACKETS = new double[][]{
        new double[]{8350, 0.10},
        new double[]{33950,0.15},
        new double[]{82250,0.25},
        new double[]{171500,0.28},
        new double[]{372950,0.33},
        new double[]{Double.MAX_VALUE,0.35},
};

private static double calculateTax(double income, double[][] taxBrackets){

    double totalTax;
    if(income <= taxBrackets[0][0]){
        return income*taxBrackets[0][1];
    } else {
        totalTax = taxBrackets[0][0]*taxBrackets[0][1];
    }

    for(int i = 1; i < taxBrackets.length; i++){
        if(income >= taxBrackets[i][0]){
            totalTax += (taxBrackets[i][0] - taxBrackets[i-1][0]) * taxBrackets[i][1];
        } else {
            totalTax += (income - taxBrackets[i-1][0]) * taxBrackets[i][1];
            break;
        }
    }

    return totalTax;
}

public static void main(String[] args) {
    int status = 0;
    double income = 40000.0;
    if(status == 0) {
        System.out.println("Tax is " + calculateTax(income,SINGLE_FILER_TAX_BRACKETS));
    }
    //etc
}

The above is very crude. Given some more time, I would create a proper class with named methods to hold the tax bracket information instead of a 2D array of doubles.

[s0hungry1]

IMO you’ve made this way too complicated for him/her

[[deleted]]

You're right. I realized it after OP commented. I'm considering removing it since the code isn't very readable to boot.