19
\$\begingroup\$

I recently created this Income Tax calculator based off your filing status (Single, or Married Filing Jointly) and which tax bracket you fall into for the 2020 tax year, taking into account the Standard deduction. I know the code is a little robust, but the math checks out as compared to several other tax calculators. I first posted it on Stack Overflow, which I now know was not the place for this, but please give any constructive advice you have.

How can I make this cleaner, as in, accomplish the same task with less code? Or if you have a different/more efficient way of writing any of this, I'm also interested. Update: Thank you everyone for your advice and input! I'll be taking this back to the whiteboard and making the suggested changes as I learn more. Your time and advice is appreciated, immensely!

marital_status = input("Filing Status (M / S): ")
household_income = int(input("household income: "))


if marital_status == "M":
    standard_deduction = "24800"
    # marrital standard deduction
elif marital_status == "S":
    standard_deduction = "12400"
    # single standard deduction


taxable_income = int(household_income) - int(standard_deduction)


married_bracket_caps = [19750, 80250, 171050, 326600, 414700, 622050]
    # the above rates are the max dollar amount in the Married-filing-joint tax bracket(m/j)
single_bracket_caps = [9875, 40125, 85525, 163300, 207350, 518400]
    # the above rates are the max dollar amount in the filing Single tax bracket

if marital_status == "M":
    bracket = married_bracket_caps
elif marital_status == "S":
    bracket = single_bracket_caps
#if M is entered, the future tax rates will abide by the m/j tax rates; #if
    # S is entered than the taxt rates will abide by the Filing Single tax rates.
    #   ie. enter: M, the first tax rate will be "19750", computed as




if taxable_income <= int(bracket[0]):
    tax = taxable_income * .1
elif taxable_income <= int(bracket[1]):
    tax = (int(bracket[0]) * .1)+(.12*(taxable_income - int(bracket[0])))
elif taxable_income <= int(bracket[2]):
    tax = (int(bracket[0]) * .1)+(.12*(int(bracket[1]) - int(bracket[0])))+(.22 * (taxable_income - int(bracket[1])))
elif taxable_income <= int(bracket[3]):
    tax = (int(bracket[0]) * .1)+(.12*(int(bracket[1]) - int(bracket[0])))+(.22 * (int(bracket[2]) - int(bracket[1])))+(.24 * (taxable_income - int(bracket[2])))

print("taxable income: ", taxable_income)
print("owed in Federal tax: ", tax)

Note: The brackets for this calculator only go up four brackets because if I didn't want to do all the way up if I was going to need to rewrite it anyway. Again, thanks!

\$\endgroup\$
7
  • \$\begingroup\$ Is it really 12 in the last two branches, or should it be .12? \$\endgroup\$ – vnp Apr 27 '20 at 21:27
  • \$\begingroup\$ cleaner in which sense exactly?. Can you elaborate please? \$\endgroup\$ – πάντα ῥεῖ Apr 27 '20 at 21:30
  • \$\begingroup\$ @vnp You're exactly right. I accidentally deleted the "." while editing. A lesson in oversight. Thanks! \$\endgroup\$ – Erik Apr 27 '20 at 21:35
  • \$\begingroup\$ @πάνταῥεῖ The math that's done in each bracket is correct (except for what VNP pointed out), but I'm wondering if I can't accomplish the same with less written code. \$\endgroup\$ – Erik Apr 27 '20 at 21:35
  • 5
    \$\begingroup\$ @Erik Also note that less lines of code doesn't automatically means cleaner. In fact it's often the opposite. \$\endgroup\$ – πάντα ῥεῖ Apr 27 '20 at 21:50
28
\$\begingroup\$

You have 1 reputation point, and you make no use of functions or complex data structures. Because of this, I'm going to assume you're a beginner to coding.

Congratulations, beginner, on finishing a program and on submitting it for review! You will find that you can learn a huge amount from reviews, probably more stuff faster than by any other method.

Control & Data structures

You don't show any loops or functions, so I won't suggest using them. Yet.

PEP 8

I will suggest that you take a look at PEP-8. It's "THE standard" for writing Python code, so the sooner you start drinking the Kool-Aid, the sooner you'll stop getting criticized for not following it.

CAPS are for constants

One thing you'll find in PEP 8 is the notion of different letter-case conventions for different kinds of name. Variable names that change are generally in what is called snake_case, while "variables" that don't change (that are called "constants" despite the fact that you could change them) are spelled using ALL_CAPS.

In your code, you have some symbols that meet these criteria. Specifically, married_bracket_caps and single_bracket_caps are constants, and should be spelled that way.

You can argue about whether other symbols, that are initialized based on a single choice and never change later, are constant or not. And people do. ;-)

Error Checking

You don't check for errors. At this point, with no way to loop and try again, your only option would be to exit the program. But you should still get in the habit of validating your inputs, especially inputs that come from those pesky humans. Humans are notorious for providing bad inputs. You have this:

if marital_status == "M":
    standard_deduction = "24800"
    # marrital standard deduction
elif marital_status == "S":
    standard_deduction = "12400"
    # single standard deduction

Add this:

else:
    print("Eat flaming death, vile Terry!")
    sys.exit(1)

(Or some other message, if you'd like.)

Don't Repeat Yourself

There is a currently-popular "principle" of programming called "DRY" -- short for "Don't Repeat Yourself." A good programmer has dry code, dry socks, and a dry wit.

In your case, you are repeating a test:

if marital_status == "M":
    standard_deduction = "24800"
...    

taxable_income = int(household_income) - int(standard_deduction)

if marital_status == "M":
    bracket = married_bracket_caps
...

Instead of repeating yourself, move the check down until you have all the data you need, and then do the test one time:

if marital_status == "M":
    bracket = MARRIED_BRACKET_CAPS
    standard_deduction = MARRIED_STANDARD_DEDUCTION

DRY your code, redux

Also, you have this:

elif taxable_income <= int(bracket[1]):
    tax = (int(bracket[0]) * .1)+(.12*(taxable_income - int(bracket[0])))

This is bad because it repeats a lot, and because it puts too much work in a single expression. Let's look at it:

tax = bracket-0-cap * some rate + (income - bracket-0-cap) * some other rate

You can dry this out by eliminating a lot of the calls. For example, why are you calling int() on numbers that YOU put into YOUR array as integers? They're already ints, you don't need to do that!

No Magic Numbers

What's .1? What's .12? Those are called magic numbers and they're bad.

Use constants (which you now know should be in ALL_CAPS) to replace these. Or use a lookup of an array (which is constant). Like:

tax = RATE_0 * income

or

tax = TAX_RATES[0] * income
\$\endgroup\$
2
  • 3
    \$\begingroup\$ You hit the nail on the head Austin! Very beginner (2-3 weeks in). I've found the PEP 8 and am already starting to read through it (I'll continue to study it). I've heard/read the suggestions about DRY and error checking. Thank you so much for your input, I'll definitely be incorporating your lessons. And I hope Terry meets his flaming end. Thank you for that laugh. \$\endgroup\$ – Erik Apr 28 '20 at 1:01
  • 2
    \$\begingroup\$ @Erik the Terries are us! It's a 60's sci-fi reference, uttered by the slime-fingered Groaci, Jaime Retief's eternal opponents in the Corps Diplomatique Terrestrienne. \$\endgroup\$ – aghast Apr 29 '20 at 1:10
9
\$\begingroup\$

The main principle here is DRY (Don't Repeat Yourself). The trickiest part of your code is where you're successively applying the tax rates to the different brackets; a lot of the code is copied and pasted, including the rates themselves. Ideally you'd want to specify those rates in exactly one place and then have reusable code that's able to apply them in different situations. For a data-driven app like this, I think it's better to define all the data in one place in a set of tables rather than have it embedded throughout the rest of the code.

Here's what I came up with after pulling the data from your code out into a set of lists, dicts, and enumerations:

from enum import Enum


class FilingStatus(Enum):
    """Tax filing status."""
    MARRIED = "M"
    SINGLE = "S"


STANDARD_DEDUCTION = {
    # Standard deduction by filing status.
    FilingStatus.MARRIED: 24800,
    FilingStatus.SINGLE: 12400,
}
# Tax rates for income up to each bracket cap.
BRACKET_TAX_RATES = [.1, .12, .22, .24, .32, .35, .37]
# Bracket caps by filing status.
BRACKET_CAPS = {
    FilingStatus.MARRIED: [
        0, 19750, 80250, 171050, 326600, 414700, 622050
    ],
    FilingStatus.SINGLE: [
        0, 9875, 40125, 85525, 163300, 207350, 518400
    ],
}

assert all(
    len(bracket) == len(BRACKET_TAX_RATES)
    for bracket in BRACKET_CAPS.values()
)

filing_status = FilingStatus(input("Filing Status (M / S): "))
gross_income = int(input("household income: "))
taxable_income = gross_income - STANDARD_DEDUCTION[filing_status]

tax = 0.0
caps = BRACKET_CAPS[filing_status]
for i in range(len(BRACKET_TAX_RATES)):
    rate = BRACKET_TAX_RATES[i]
    if i+1 >= len(caps) or taxable_income < caps[i+1]:
        # Income is under the next cap (or we're at the highest one already).
        # Apply the current rate to the income above the current cap and stop.
        tax += rate * (taxable_income - caps[i])
        break
    else:
        # Income is at or above the next cap.
        # Apply the current rate to the diff of the next and current caps.
        tax += rate * (caps[i+1] - caps[i])

print("taxable income: ", taxable_income)
print("owed in Federal tax: ", tax)
\$\endgroup\$
5
  • \$\begingroup\$ Thanks Samwise! I see the changes, and please correct me if I'm wrong, but if the tax rates were to change than I would only need to change the BRACKET_TAX_RATES[ ] without having to make changes throughout the app. Thank you for your time and advice! \$\endgroup\$ – Erik Apr 28 '20 at 0:41
  • 3
    \$\begingroup\$ Yep, that's the idea! The assert is to make sure that if you change the number of brackets in one place you remember to do it in other places; you might end up with a subtle bug otherwise. \$\endgroup\$ – Samwise Apr 28 '20 at 0:44
  • 3
    \$\begingroup\$ instead of looping over the index (for i in range(len(BRACKET_TAX_RATES))), I would zip the brackets and rates in reverse order, and iterate over those. Doing it in reverse allows you to simplify the edge case. also refactor this part into a separate function: def tax_calculation(gross_income, caps, rates): tax = 0; for cap, rate in zip(reversed(caps), reversed(rates), ): income_in_bracket = gross_income - cap; tax += income_in_bracket * rate; gross_income = min(gross_income, cap); return tax \$\endgroup\$ – Maarten Fabré Apr 28 '20 at 10:18
  • 1
    \$\begingroup\$ Although there's no point in editing this answer, given @MaartenFabré's comment, in general, for i in range(len(X)): x = X[i] is better expressed as for i, x in enumerate(X):. \$\endgroup\$ – PiCTo Apr 29 '20 at 8:24
  • 1
    \$\begingroup\$ IMO an exception is when you need to manipulate i in some way, since I find an expression involving (X[i] and X[i+1]) clearer than one involving (x and X[i+1]), according to the "make alike look alike" principle -- but I understand some have strong feelings about this. :) \$\endgroup\$ – Samwise Apr 29 '20 at 14:24
4
\$\begingroup\$

You could move the declaration of married_bracket_caps and single_bracket_caps to the beginning of the code, and then combine the first couple of if/elif blocks as they are both if marital_status == 'M', etc.

\$\endgroup\$
1
  • \$\begingroup\$ Right! I basically repeated myself. Thank you Aiden! \$\endgroup\$ – Erik Apr 28 '20 at 0:44
1
\$\begingroup\$

I'm not a Python dev, so I'm going to focus specifically on one area: the mathematical calculations themselves.

Okay, so if you're making $X - how do you calculate your taxes? You're taking the 'Figure Out Which Bracket You Are In and then Calculate for that specific bracket' approach, but how about this (and I apologize for any syntax issues; like I said, it's not my language.):

alreadyTaxedIncome = 0
remainingIncome = taxable_income
taxesDueFromPriorSegments = 0

if remainingIncome <= int(bracket[0]):
    return taxesDueFromPriorSegments + remainingIncome * .1
alreadyTaxedIncome = bracket[0]
remainingIncome = taxable_income - bracket[0]
taxesDueFromPriorSegments = taxesDueFromPriorSegments + (bracket[0]-0) * 0.1

if remainingIncome <= int(bracket[1]):
    return taxesDueFromPriorSegments + remainingIncome * .12
alreadyTaxedIncome = bracket[1]
remainingIncome = taxable_income - bracket[1]
taxesDueFromPriorSegments = taxesDueFromPriorSegments + (bracket[1]-bracket[0]) * 0.12

if remainingIncome <= int(bracket[2]):
    return ... etc, repeating the above segment of 5 lines for each bracket

See what I did there? I redid the logic a bit so that it's "flattened" - if you have 20 brackets, you're not going to have an IF clause that results in a calculation that's 20 terms long. Instead, it just figures out how the taxes apply for the specific bracket, and continues going through until you've reached a bracket that no longer applies.

Not only does this simplify the logic, it makes it possible to loop. Because... why wouldn't you have a loop? After all, that segment of 5 lines is going to be repeated over and over again, differing only in the array slot you're pulling from bracket[] and the marginal tax rate for that bracket.

Next up would be OOP principles and refactoring code. In terms of objects, you've got two main ones:

TaxStructure - consists of a sorted array of TaxBrackets
TaxBracket - consists of a top income and marginal tax rate

... and you're going to want a function that takes in a TaxStructure and an income amount, and returns back the taxes owed.

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