Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my first code. I would really appreciate a critique for it, if anyone can spare the time.

def wedcost(hours):

    print " "

    try:
        hours = int(hours)
    except ValueError:
        print "Invalid input"
        new_hours = raw_input("Please use only numeric characters. ")
        hours = int(new_hours)
        print " "

    tcost = hours * 75 # $75/hr for two shooters
    if hours < 4:
        tcost = 250 # price starts at a fixed $250 from 1 to 3 hours
    if hours >= 6:
        tcost = hours * 100 # I don't like long shifts

    print "The total cost of our services will be $" + str(tcost) + "."

    print " "

    def recalc(answer): # this second function is where I had the most trouble

        outcome = 0 # only method I could get to work properly
        if answer.startswith('y' or 'Y'): 
            print " "
            outcome += 1
            wedcost(raw_input("How long will we be on location? "))
        if answer.startswith('n' or 'N'):
            print " "
            print "Have a nice day :)"
            outcome += 1
        if outcome == 0:
            print "Invalid input"
            recalc(raw_input("Would you like to recalculate? ('Yes' or 'No') "))

        # I had previously tried checking 'answer' to items in lists

    recalc(raw_input("Would you like to recalculate? ('Yes' or 'No') "))

wedcost(raw_input("How long will we be on location? "))

I'm sure that there's a better way to go about this task. Any suggestions or hints?

share|improve this question
2  
# only method I could get to work properly -- Is this code functional? –  nhgrif Aug 2 at 1:07
    
@nhgrif Yes. Well, it works when I run it in IDLE. –  SCK Aug 2 at 1:23
    
Hi, and welcome to Code Review, and congrats on breezing through the first-post review queue. –  rolfl Aug 2 at 1:56
    
@rolfl Thanks :) –  SCK Aug 2 at 2:10
1  
To me the big problem with this is that it would just be so much more convenient to use as a half dozen (or so) cells in a spreadsheet. –  Jerry Coffin Aug 2 at 3:38

1 Answer 1

up vote 6 down vote accepted

Well, this code has a few problems:

  1. Typing non-numbers twice in a row causes a ValueError.

  2. It is not quite PEP-8 compliant (the official python style guide)

  3. No docstrings

  4. Final bit of code should be wrapped in a if __name__ == "__main__": block to add reusability.

  5. It is not very reusable


Now let's fix all that:

Wrap your input section in it's own functions like this:

def get_num(prompt, errorMsg = "I'm sorry, that's not a valid number"):
    """Returns a number from the user"""
    while True:
        try:
            input = int(raw_input(prompt))
        except ValueError:
            print errorMsg
        else:
            return input

def get_yes_no(prompt, errorMsg = "I'm sorry, I didn't catch that. Could you try again?"):
    """Returns True for yes, False for no"""
    while True:
        input = raw_input(prompt)
        if input.startswith('y' or 'Y'):
            return True
        elif input.startswith('n' or 'N'):
            return False
        print errorMsg

This allows you to have more robust and more readable code

Naming: function names should be lowercase and separated by _. So wedcost(hours) becomes wed_cost(hours).

Also your function doesn't do what is advertised, since it keeps on recalculating.

Let's wrap that into a main function like this:

def main():
    wed_cost(get_num("How long will we be on location? "))
    while get_yes_no("Would you like to recalculate? ('Yes' or 'No') "):
        wed_cost(get_num("How long will we be on location? "))
    print "Have a nice day :)"

Then we can add this little bit of code to the bottom:

if __name__ == "__main__":
    main()

Which will run your code if it is directly called, but also allows you to reuse this code elsewhere with an import without causing any code to be run.


Finally let's put all your magic numbers (such as 75, 100 etc) somewhere more recyclable by defining some constants at the top of the code:

MIN_FEE = 250
HOURLY_FEE = 75
OVERTIME_HOURS = 6
OVERTIME_FEE = 100

Then the wed_cost function becomes:

def wed_cost(hours):
    """Prints a message with the cost of a wedding shoot given a number of hours"""
    totalCost = 0
    if hours >= OVERTIME_HOURS:
        totalCost = hours * OVERTIME_FEE
    totalCost = max(hours * HOURLY_FEE, MIN_FEE)
    print "The total cost of our services will be ${}.".format(totalCost)

This makes your code more understandable, and more debuggable :)

share|improve this answer
    
Thank you so much. That is an unexpectedly detailed and helpful response. Cheers! –  SCK Aug 2 at 3:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.