3
\$\begingroup\$

This is a calculator I'm working on. The ultimate goal is to calculate multiple payrolls before selecting to end the program, please review current version in that context. PSA: I'm a beginner.

def main():

    hours, rate = inputData()
    normal_hours, overtime_hours, overtime  = computePay (hours,rate)
    regular, total_pay  = preovertime_hours (hours, rate, overtime)
    displayPay  (rate, normal_hours, overtime_hours, regular, overtime, total_pay)


def inputData():

    name = raw_input('Name of employee: ')    
    hours = float (input('Now enter the hours worked please:'))
    rate = float (input('Now enter the pay rate:'))


    return hours, rate

def computePay (hours,rate):

    if hours < 40:
        normal_hours = hours
        overtime_hours = 0
    else:
        normal_hours = 40
        overtime_hours = hours - 40

    if hours > 40:
        overtimerate = 1.5 * rate
        overtime = (hours-40) * overtimerate
    else:
        overtime = 0

    return normal_hours, overtime_hours, overtime

def preovertime_hours (hours, rate, overtime):

    regular = hours * rate
    total_pay = regular
    return regular, total_pay


def displayPay (rate, normal_hours, overtime_hours, regular, overtime, total_pay):

    print ("The amount to be paid to in this pay period is $" + format(total_pay, '.2f'))

main()
\$\endgroup\$
  • 2
    \$\begingroup\$ "The goal is to calculate multiple payrolls before selecting to end the program." -- sounds like something you want to implement, but missing in the posted code. Our goal here is to review fully working code and suggest improvements, not to implement missing features. I suggest to rephrase that part, because it's not clear what you're looking for here \$\endgroup\$ – Stop ongoing harm to Monica Nov 6 '15 at 7:53
  • \$\begingroup\$ Voted to reopen as I disagree with the 'obviously broken code'. The phrase in the title may be a bit confusing, but I see it as "my plan is to ... so that you can understand why I wrote this code". I can see possibly unintended bug (or something against coding rules which may actually work, but looks bad), so, worth reviewing. \$\endgroup\$ – user52292 Nov 6 '15 at 10:07
  • 3
    \$\begingroup\$ Actually, after looking at the edits made to the post, you should understand that we are not ever smiling saints hungry to feel helping and that police is here to save our time. Your post was not so bad (you'll get downvoted otherwise), but you should put more effort in it next time. Give respect and effort, receive respect and effort ;) \$\endgroup\$ – user52292 Nov 6 '15 at 10:22
2
\$\begingroup\$

First off, there currently is no point in having name = raw_input('Name of employee: '), as it is never used.

Generally it's safer to never use input. For example:

>>> hello, world = 'goodbye', 'logic'
>>> input()
hello + world
'goodbyelogic'

As you can see, input evaluates the input. Which is not very safe. Lets say you have some special information that no-one should know about. But you stored it in a variable. We with our 'elite haxor skillz', we could find out that information. for example:

>>> private_key = 'my super private key'
>>> input()
globals()['private_key']
'my super private key'

Bottom line input is bad, so bad that it got removed in Python3. And so, I would recommend to always use raw_input, in Python2.

float (input('Now enter the hours worked please:'))

First as mentioned above, this should be raw_input, but it is flimsy, say we input a. float will error, and the program will exit.

Instead you can use a try except statement.

while True:
    try:
        float(input('Now enter the hours worked please:'))
        break
    except ValueError:
        pass

computePay is really good. If you are ok with using turnery operators you can re-write it, but your way is probably best. Except for overtimerate, that should be part of overtime. If I were to re-write your code then it would be something like:

def computePay (hours,rate):
    normal_hours, overtime_hours = hours, 0 if hours < 40 else 40, hours - 40
    overtime = (hours - 40) * 1.5 * rate if hours > 40 else 0
    return normal_hours, overtime_hours, overtime

preovertime_hours can just be a return statement. You could even opt for it not to be a function. I'm more inclined for the latter as it just returns hours * rate, hours * rate. Also you should add it to computePay as it would fit better there.

In displayPay you may want to use str.format rather than format. They're very similar, so to change to str.format you would do:

"The amount to be paid to in this pay period is ${:.2f}".format(total_pay)

As for main. Due to all the changes above I personally would recommend that you change it to 3 function calls. They are inputData, computePay and displayPay. Whilst you could merge it all into one function and keep everything simple, keeping IO away from logic is always good.

Overall your structure is pretty good. There are a lot of nice small functions.


Just to note, you should always have functions declared and used as function(). Note there is no space between the name and the brackets!

Also as some languages allow - in names, it's always a good idea to put a space either side of operators.

You could 'wrap' your main() in a if __name__ in '__main__':. This prevents your code running if it's not the main file. (E.g. if someone imports your file into there code.)

But otherwise your code is good.


Just so you know currently almost all your code is dead code (It does nothing). And so you could re-write main into:

def main():
    hours, rate = inputData()
    displayPay('', '', '', '', '', hours * rate)

The ultimate goal is to calculate multiple payrolls before selecting to end the program

If you want this you could make a while loop like the example above to fix float(input()). A simple way to do this is to ask a question like 'Would you like to add another user? [y/n] '. And exit on any input that is not 'y' or 'yes'.

As I'm not going to write it for you, the following is half the loop.

if input('Would you like to add another user? [y/n] ').lower() in ['y', 'yes']:
    main()
\$\endgroup\$
  • \$\begingroup\$ Thank you for your input @Joe Wallis, I appreciate it! Let me work on some more stuff and maybe I will post again. \$\endgroup\$ – Aaron Nov 6 '15 at 16:38
  • \$\begingroup\$ @Xarotic No problem, also be careful you're not allowed to change the code in the question. Just in-case you are thinking of doing that (I get the feeling you are) \$\endgroup\$ – Peilonrayz Nov 6 '15 at 16:41
  • \$\begingroup\$ would you be open to a chat? I would love to ask some questions about my program, but I feel the comment section isn't the place to do so. \$\endgroup\$ – Aaron Nov 6 '15 at 16:49
  • \$\begingroup\$ No problem, just post it here! \$\endgroup\$ – Aaron Nov 6 '15 at 17:53
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – Peilonrayz Nov 6 '15 at 18:17
2
\$\begingroup\$

I mostly agree with @Joe Wallis, but I'd rewrite your computePay function like this:

def compute_pay (hours,rate):
    overtime_rate = 1.5 * rate
    normal_hours = min(hours, 40)
    overtime_hours = max(hours-40, 0)
    overtime = overtime_hours * overtime_rate
    return normal_hours, overtime_hours, overtime

It's short and nice, and I can't get why use ternary operators when you can just use max.

\$\endgroup\$
  • 1
    \$\begingroup\$ I was going to say you can use min and max. But I think the intent is harder to understand. just like bool(x) ^ bool(y) is the same as bool(x) != bool(y) but the latter is more understandable/understood. \$\endgroup\$ – Peilonrayz Nov 6 '15 at 18:04
  • \$\begingroup\$ Well, I think in this case min and max are pretty understandable. The specification would probably say something like "Normal hours is 40 hours or time worked whichever is less" or "normal hours is time worked but no more than 40 hours". Both are literally translated as min(hours, 40). \$\endgroup\$ – Alissa Nov 6 '15 at 18:10
  • \$\begingroup\$ I didn't mean to come across as 'your wrong!' So sorry if I did. I don't disagree that min and max are both pretty understandable, I just don't think they are as understandable. Just for the same reasons as the example above. As most people go 'what?!' when they see that for the first time, or at-least everyone I know. \$\endgroup\$ – Peilonrayz Nov 6 '15 at 19:04

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.