Skip to content

Instantly share code, notes, and snippets.

@atenni
Last active August 15, 2020 16:40
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save atenni/7dac61518d96fb6431edf02bf036a878 to your computer and use it in GitHub Desktop.
Save atenni/7dac61518d96fb6431edf02bf036a878 to your computer and use it in GitHub Desktop.
Code review for colleague

Overall this is pretty impressive for someone just starting out - well done!

Here's a few overall design comments. I'll include technical feedback as comments directly in Updated-PythonPractice.py

DRY Principle - Don't Repeat Yourself.

When coding you don't want to write the same piece of functionality twice. In longer programs this type of duplication makes it hard to follow the flow of the program, and it also creates situations where it's really hard to track down bugs. You think you've fixed it here, but you've got the same issue over there 100 lines down the page...

In your Original-PythonPractice.py you can avoid this by taking lines 6-14, and 31-39 outside of your if statment (see Updated-PythonPractice.py for an example of this). A side benefit is that you end up with less code, and less code is usually simplier and easier to understand :)

Variable names

Most of your variable names are good except for y and z. You want your variables to describe what they are so that when you come across them 100 lines down the page, or in 6 months time when you need to update the program, it's easy to understand what they're doing. y and z don't tell me anything.

In Updated-PythonPractice.py I've changed these to daily_hours and days_per_week. This also makes the maths that calculates the weekly salary a lot more readable.

Program flow

What happens if someone types Yes to your first question? Probably not the result you want... Have a look at the code and try to work it out before running it to see.

There's several ways you could tighten this up, but the easiest is probably changing the else on line 30 of Original-PythonPractice.py to an elif ....

(Bonus points if you add a little code that checks if the user's input matches the A or B you're expecting. If not, show them a helpful message and ask again.)

Multiline strings

If you want to include multiline strings in Python you can use triple quotes: ''' or """. These also work with f-strings. See line 4 of Updated-PythonPractice.py for an example.

Potential bug

In your tax elif logic what happens if, for example, a resident's income works out to be $18,000.50? How can you fix these statements?

print("This is to calculate your weekly salary, yearly salary & amount of tax you have to pay the ATO")
print("-" * 30)
resident_status = str(input("Are you a resident? \nA: Yes \nB: No \n"))
if resident_status == "A":
hourly_rate = float(input("Enter Your Hourly Rate:"))
y = float(input("Enter your daily work hours: "))
z = float(input("Enter number of work days a week: "))
weekly_salary = hourly_rate * y * z
print(f"Your weekly salary is: ${weekly_salary}")
yearly_salary = weekly_salary * 52
print(f"Your yearly salary is: ${yearly_salary}")
if yearly_salary <= 18200:
print("No Tax")
elif 18201 <= yearly_salary <= 37000:
tax_1 = (yearly_salary - 18200) * 19 / 100
print(f"Your tax to the ATO is: ${tax_1}")
elif 37001 <= yearly_salary <= 90000:
tax_2 = (yearly_salary - 37000) * 32.5 / 100 + 3572
print(f"Your tax to the ATO is: ${tax_2}")
elif 90001 <= yearly_salary <= 180000:
tax_3 = (yearly_salary - 90000) * 37 / 100 + 20797
print(f"Your tax to the ATO is: ${tax_3}")
else:
tax_4 = (yearly_salary - 200000) * 45 / 100 + 51592
print(f"Your tax to the ATO is: ${tax_4}")
else:
hourly_rate = float(input("Enter Your Hourly Rate:"))
y = float(input("Enter your daily work hours: "))
z = float(input("Enter number of work days a week: "))
weekly_salary = hourly_rate * y * z
print(f"Your weekly salary is: ${weekly_salary}")
yearly_salary = weekly_salary * 52
print(f"Your yearly salary is: ${yearly_salary}")
if yearly_salary <= 90000:
tax_1 = yearly_salary * 32.5 / 100
print(f"Your tax to the ATO is: {tax_1}")
elif 90001 <= yearly_salary <= 180000:
tax_2 = (yearly_salary - 90000) * 37 / 100 + 29250
print(f"Your tax to the ATO is: ${tax_2}")
else:
tax_3 = (yearly_salary - 200000) * 45 / 100 + 62550
print(f"Your tax to the ATO is: ${tax_3}")
print("This is to calculate your weekly salary, yearly salary & amount of tax you have to pay the ATO")
print("-" * 30)
# Converted this statement to a multiline string
resident_status = str(input("""Are you a resident?
A: Yes
B: No
"""))
hourly_rate = float(input("Enter Your Hourly Rate: ")) # Minor: added space after colon
hours_per_day = float(input("Enter your daily work hours: "))
days_per_week = float(input("Enter number of work days a week: "))
weekly_salary = hourly_rate * hours_per_day * days_per_week
print(f"Your weekly salary is: ${weekly_salary}") # Excellent use of f-strings!
yearly_salary = weekly_salary * 52
print(f"Your yearly salary is: ${yearly_salary}")
if resident_status == "A":
if yearly_salary <= 18200:
print("No Tax")
elif 18201 <= yearly_salary <= 37000:
tax_1 = (yearly_salary - 18200) * 19 / 100
print(f"Your tax to the ATO is: ${tax_1}")
elif 37001 <= yearly_salary <= 90000:
tax_2 = (yearly_salary - 37000) * 32.5 / 100 + 3572
print(f"Your tax to the ATO is: ${tax_2}")
elif 90001 <= yearly_salary <= 180000:
tax_3 = (yearly_salary - 90000) * 37 / 100 + 20797
print(f"Your tax to the ATO is: ${tax_3}")
else:
tax_4 = (yearly_salary - 200000) * 45 / 100 + 51592
print(f"Your tax to the ATO is: ${tax_4}")
else:
if yearly_salary <= 90000:
tax_1 = yearly_salary * 32.5 / 100
print(f"Your tax to the ATO is: ${tax_1}") # Minor: added dollar sign
elif 90001 <= yearly_salary <= 180000:
tax_2 = (yearly_salary - 90000) * 37 / 100 + 29250
print(f"Your tax to the ATO is: ${tax_2}")
else:
tax_3 = (yearly_salary - 200000) * 45 / 100 + 62550
print(f"Your tax to the ATO is: ${tax_3}")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment