Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Ok I've started programming 3 days ago. I decided to choose python3 as my language because its pretty easy to learn yet reasonably powerful. I've used a variety of resources on the net, most of which were originally made for python2. I've used my new found skills to make a simple program that can calculate the area of various 2d shapes. I'd like your expereanced eyes to asses my code. Be as harsh as you want but please be specific and offer me ways in which I can improve my coding. Heres the code:

shape = ("triangle","square","rectangle","circle")                  # This creates a tuple containing the string names of all the shapes to be calculated . 
constants = (3.14)                                                  # This stores the value of pi. Later I will add more constants to it and make it a tuple.
start_up = input("Do you want to calculate a shape's area?: ")      # Fairly self explanatory.

if start_up != ("yes" or "no"):                                     # This is to prevent the program failing if the user answers the start_up question.
   print("Sorry but I don't understand what your saying")           # with an answer outside of the simple yes/no that it requires.
   start_up = input("Do you want to calculate a shape's area?: ")   # re-iterates the start up question.
   start_up = input("Do you want to calculate a shape's area?: ")   # re-iterates the start up question.

while start_up == "yes":                                            # This is the condition that obviously contains the bulk of the program.
      target_shape = input("Choose your shape: ")                       # Once again, self explainatory.

      if target_shape == shape[0]:                                      # Uses the shape tuple to bring forth the case for the first shape, the triangle.
         h = float(input("give the height: "))                          # These Two lines allow the user to input the required values to calculate the area of the triangle.
         b = float(input("give the base length: "))                     # They are using the float funtion to increase the accuracy of the answer.
         area_triangle = h * 1/2 * b                                    # The formula we all learned in school.    
         print("the area of the %s is %.2f" % (shape[0],area_triangle)) # This generates a format string which uses both the shape tuple and the area_triangle object 
                                                                    # to display the answer. note the %.2f operand used to specify how many decimal places for the answer.

      if target_shape == shape[1]:                                      # Square
         l = float(input("give the length: "))                          
         area_square = l ** 2
         print("the area of the %s is %.2f" % (shape[1],area_square))

      if target_shape == shape[2]:                                     # Rectangle
         l = float(input("give the length: "))
         w = float(input("give the width: "))
         area_rectangle = l * w
         print("the area of the %s is %.2f" % (shape[2],area_rectangle))

      if target_shape == shape[3]:                                     # Circle
         r = float(input("give the radius: "))
         pi = constants                                                # In this line we are calling forth the constants object to use the value of pi.
         area_circle = 2 * pi * r
         print("the area of the %s is %.2f" %(shape[3],area_circle))

         start_up = input("Do you want to calculate another shape's area?: ") # This line allows the user the chance to do just what it says in the brackets.

      else:                                                                   # This is to safegaurd against failure in the event of the user  
          print("That shape's not in my database")                             # inputing a shape non-existant in the shape tuple.
          start_up = input("Do you want to calculate another shape's area?: ")

if start_up == "no":                                                      #This is for when the user no longer wants to use the program.
   print("Goodbye then!")
   input("Press any key to close")

Edit: Ok thanks for all the helpful comments. Here's my revised code. I've tried to take on board the advice of everyone to the best of my ability.

while True:
    startup = None
    while startup not in ("yes", "no"):
        if startup is not None:
            print("Sorry but I don't know what you are saying.")
        startup = input("Do you want to calculate a shape's area?: ").lower()
    if startup == "no":
        print("Goodbye then!")
        input("Press enter to close")
        break
    elif startup == "yes":
        print("Cool!")
        shape = None
        while shape not in ("triangle", "square", "rectangle", "circle"):
            if shape is not None:
                print("""sorry but I don't know that shape.
That is if it even is a shape. -_-""")
            shape = input("What shape?: ").lower()
            if shape == "triangle":
                print("Ok you need to give me the height and the base length")
                h = float(input("height = ")); b = float(input("base = "))
                area = h * 1/2 * b
            elif shape == "square":
                print("Ok I just need the length of a side from you")
                l = float(input("length = "))
                area = l ** 2
            elif shape == "rectangle":
                print("give me the length and width")
                l = float(input("length = ")); w = float(input("width = "))
                area = l * w
            elif shape == "circle":
                print("I just need the radius")
                r = float(input("radius = "))
                area = 2 * 3.14 * r
        print("The area = "); print(area)

Note that there is one huge problem in this code that I still haven't resolved. As Justin had pointed out, if someone types in an invalid value or even nothing at all for the area formulas, the program will fail. I've tried to fix this but I can't. Any help here would be great. Also I tried to substitue 3.14 with math.pi like Cygal suggested but it seemed to make my program no longer work. I'm pretty sure I'm just using it wrong.

share|improve this question
Well I actually want to find out how to optimise my code. I'm not sure how to best use things like tuples to accomplish this. – RationalMystic Sep 13 '12 at 13:56
That isn't what optimization means, although the code you pasted won't work at all. – Wooble Sep 13 '12 at 13:56
Also, why did you edit "python3" to "python2" in the body of the question but not the title and tags? Which version are you actually using? – Wooble Sep 13 '12 at 13:57
1  
Copy the Code. Open the Question window and paste it. Highlight all of it and click the {} button. It really couldn't be easier. And no, it doesn't work as intended, at the very least if start_up != "yes" or "no": is always true. – Wooble Sep 13 '12 at 14:06
1  
The if start_up != ("yes" or "no"): is still wrong; ('yes' or 'no') is 'yes'. You're looking for not in or 2 inequality tests. – Wooble Sep 13 '12 at 16:55
show 2 more comments

migrated from stackoverflow.com Sep 13 '12 at 13:57

3 Answers

up vote 3 down vote accepted

In addition to Justin's comments:

You want to be more DRY. You have the line:

start_up = input("Do you want to calculate a shape's area?: ")

in your code 5 times (one of which is just completely broken because regardless of the answer it's asked again) when once would suffice:

while True:
    startup = None
    while startup not in ("yes", "no"):
        if startup is not None:
            print("Sorry but I don't understand what your saying")
        startup = input("Do you want to calculate a shape's area?: ")
    if startup == "no":
        print("Goodbye then!")
        input("Press enter to close") # <-- "any key" is a lie; will require return...
        break
    # do the rest of the stuff here.

The bit that prints the area is also repeated unnecessarily; call the results for each section "area" and use that; you already insert the correct shape name.

Your indentation is also inconsistent; don't switch between 3 and 4 space indents; pick one.

The else clause at the end also only applies to the last if; any shape but circle will be calculated and the user will be told the shape isn't in the "database". replace all the ifs but the first with elif.

share|improve this answer
Man I did say be as harsh as you like but you guys are tearing my code apart. I've also started to find more and more faults in my code my self. I'm thinking of completely rewriting this program from scratch because editing it is becoming a pain. Thanks again for the input! – RationalMystic Sep 13 '12 at 17:22
I've used the above loop structure into my code with great success but I have to admit that I don't fully understand how it works. From my understanding "None" is some kind of placeholder object that doesn't have any properties and in this code is primarily used to prevent startup from continuously reiterating itself before I've even typed in an input. I don't understand the "True" object at all though. – RationalMystic Sep 14 '12 at 14:05
@RationalMystic: while True: simply creates an infinite loop which will run over and over until it reaches a break. It's used to avoid using the same code to set a loop control variable both before the loop and at its end. – Wooble Sep 14 '12 at 14:46
Ok I think I understand now. – RationalMystic Sep 14 '12 at 15:02

Since you're not using shape extensively I would get rid of that data structure all together. It would be easier to read inline then having to look to the margin for comments.

For example:

  if target_shape == 'square':                                     
     l = float(input("give the length: "))                          
     area_square = l ** 2
     print("the area of the square is %.2f" % (area_square))

When comparing strings if you don't want to be strict you should normalize them before compare.

For example:

'Square'.lower() == 'square'

so your code would become:

  if target_shape.lower() == 'square':                                     
     l = float(input("give the length: "))                          
     area_square = l ** 2
     print("the area of the square is %.2f" % (area_square))

You're not accounting for any errors. What if the user hits enter without giving a length?

ValueError: could not convert string to float
share|improve this answer
You shouldn't use is to compare strings; that tests identity, not equality. – DSM Sep 13 '12 at 15:07
touche! you are correct – Justin Sep 13 '12 at 15:10
Thank you for the valuable input. I actually was aware of the whole caps issue but I didn't know what the function was to impart uniformity on my strings. Will update my code promptly. I did account for certain errors such as when the user writes in a nonsense shape or answer, forgetting about the intra-calculation (new word :P) possibilities. Will be fixing that up too. Thanks again. – RationalMystic Sep 13 '12 at 16:47

Your code is very good for a beginner! As you have noticed, there are always improvements to be done, so don't worry too much about this. Here's what I feel the other answers have missed:

constants = (3.14)

Use math.pi instead!

start_up = input("Do you want to calculate a shape's area?: ")      # Fairly self explanatory.

I understand that those comments help you as a Python beginner, but they should be avoided in real code. As the PEP 8 section on online comments says, inline comments that state the obvious are distracting. You're also breaking the 79 characters rule.

share|improve this answer
1  
As noted in the comments above, if start_up != "yes" or "no" will not work, because that's not how or works. For similar reasons, start_up != ("yes" or "no") won't work either. – DSM Sep 14 '12 at 10:25
1  
To be specific: The "or" will return the first truthy value, so ("yes" or "no") is simply "yes". In the other case, if start_up = 'yes', then start_up != "yes" or "no" will be False or "no", which is "no", which is a nonempty string and thus truthy, so the branch will still be taken. – DSM Sep 14 '12 at 10:31
Thanks! For some reason, I was assuming there was the same magic than in 1 < x < 5. I removed the wrong bit. – Quentin Pradet Sep 14 '12 at 11:05
Thank you. I've started rewritting the code and I think I've managed to iron out most of my syntaxial mistakes. There however is one thing which I can't seem to implement. How can you safeguard against someone inputting either nothing or an invalid value when it asks you to provide the values to carry out the area equations? – RationalMystic Sep 14 '12 at 14:00
1  
You can catch the ValueError exception that float() is throws when the argument is empty or invalid, and ask for a value again. It's not really important, and if you want to do it, consider writing a function which you'll be able to call whenever you need this. eg. get_float("I just need the length of a side: "). The function will be able to loop until the user gives an useful value. By the way, you should upvote questions that helped you (not only mine!), and decide on which question helped you the most. When you've rewritten your code, you can ask another question if you want to. – Quentin Pradet Sep 14 '12 at 15:16
show 3 more comments

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.