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

def wedcost(hours):

    print " "

        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?

  • 2
    \$\begingroup\$ # only method I could get to work properly -- Is this code functional? \$\endgroup\$ – nhgrif Aug 2 '14 at 1:07
  • \$\begingroup\$ @nhgrif Yes. Well, it works when I run it in IDLE. \$\endgroup\$ – muad-dweeb Aug 2 '14 at 1:23
  • \$\begingroup\$ Hi, and welcome to Code Review, and congrats on breezing through the first-post review queue. \$\endgroup\$ – rolfl Aug 2 '14 at 1:56
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Jerry Coffin Aug 2 '14 at 3:38
  • \$\begingroup\$ @JerryCoffin Correct. The fundamental purpose of this function was to allow me to figure out a code all on my own, without being hand-held or guided by a lesson plan. I figured that I would ask this community for critique if I could get it to function somewhat decently first. I really don't have any bigger plans for it, but I will be thoroughly exploring the feedback I have received. Better to catch bad practices when they're still fresh ;) \$\endgroup\$ – muad-dweeb Aug 2 '14 at 3:52

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:
            input = int(raw_input(prompt))
        except ValueError:
            print errorMsg
            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__":

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

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 :)

  • \$\begingroup\$ Thank you so much. That is an unexpectedly detailed and helpful response. Cheers! \$\endgroup\$ – muad-dweeb Aug 2 '14 at 3:45

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.