4
\$\begingroup\$

Previous question: Decide whether a triangle is isosceles or not

Using the comments on the previous question (from people such as Carcigenicate, Luke Sawczak, dgnuff, alecxe and 200_success) I have revised the code and made an improved version but I'm sure that there is still more that I can do to improve it. (And while the calls to sleep and the 'print("")' may be unnecessary, I prefer to write my code with them to make the output look better).

import time

def askSideLength(sideMessage):
    while True:
        try:
            length = float(input("How long is the " + sideMessage + " side of the triangle? "))
            if length <= 0.0:
                print("This is an impossible triangle!")
                time.sleep(0.5)
            else:
                return length
        except ValueError:
                print("That's not a number...")
                time.sleep(0.5)

def findTriangleType():
        if side1 == side2:
            print("This is an isosceles triangle!")
        elif side1 == side3:
            print("This is an isosceles triangle!")
        elif side2 == side3:
            print("This is an isosceles triangle!")
        elif side1 != side2 and side1 != side3:
            print("This is not an isosceles triangle!")
        elif side2 != side1 and side2 != side3:
            print("This is not an isosceles triangle!")
        elif side3 != side1 and side3 != side2:
            print("This is not an isosceles triangle!")

print("""I am going to ask you for three numbers.
These numbers can be integers or decimals.
They will be the sides of a triangle,
and I will tell you if it is an isosceles triangle or not.
""")
time.sleep(2.5)
side1 = askSideLength("first")
time.sleep(0.25)
side2 = askSideLength("second")
time.sleep(0.25)
side3 = askSideLength("third")
print("")
time.sleep(1)

semiSum = (side1 + side2 + side3) / 2.0
areaSquared = semiSum * (semiSum - side1) * (semiSum - side2) * (semiSum - side3)

if side1 == side2 == side3:
    print("""While this triangle is technically an isosceles triangle,
it is much more commonly known as an equilateral triangle.""")
elif areaSquared <= 0.0:
    print("This is an impossible triangle!")
elif areaSquared > 0.0:
    findTriangleType()
time.sleep(2.5)
\$\endgroup\$
2
  • \$\begingroup\$ hi there - where are your tests? In any case i don't like the above code because it's not really an OOP way. i can provide my thoughts but it will be in c# - you'll be able to understand it perfectly though. LMK if that's ok and i'll post my thoughts later tonight. \$\endgroup\$ – BKSpurgeon Mar 30 '17 at 5:48
  • 1
    \$\begingroup\$ It looks like you really don't like combining conditions with or. If this is something that you haven't learned yet you should say so, otherwise people will keep pointing that out. \$\endgroup\$ – ChatterOne Mar 30 '17 at 6:49
6
\$\begingroup\$

Law of Excluded Middle

Something is either an isosceles triangle or it isn't. (This is a consequence of Law of Excluded Middle). Given this knowledge, as an exercise, what should you be able to eliminate from this code:

    if side1 == side2:
        print("This is an isosceles triangle!")
    elif side1 == side3:
        print("This is an isosceles triangle!")
    elif side2 == side3:
        print("This is an isosceles triangle!")
    elif side1 != side2 and side1 != side3:
        print("This is not an isosceles triangle!")
    elif side2 != side1 and side2 != side3:
        print("This is not an isosceles triangle!")
    elif side3 != side1 and side3 != side2:
        print("This is not an isosceles triangle!")

Don't sleep on me.

There really is no reason to write:

time.sleep(2.5)
side1 = askSideLength("first")
time.sleep(0.25)
side2 = askSideLength("second")
time.sleep(0.25)
side3 = askSideLength("third")
time.sleep(1)

The prompt will wait for the user to input the data, don't make the user wait.

Naming conventions

askSideLength for example should be converted to ask_side_length. We don't use the camelCase for functions and variables in Python. Instead we use snake_case.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your naming convention suggestion was my fault. I suggested that in the previous review. I haven't written Python in years, and it shows. Using snake case in Python seemed too obvious. My bad. \$\endgroup\$ – Carcigenicate Mar 29 '17 at 17:21
  • \$\begingroup\$ @Carcigenicate: No worries, I see people do it all the time. \$\endgroup\$ – Dair Mar 29 '17 at 17:23
6
\$\begingroup\$

To make the findTriangleType function more generally useful, it should

  • take parameters instead of relying on global variables
  • return a value instead of directly printing the result. The calling code can then decide what to do with the result. For example, if you turn your program into a GUI application, you would no longer use print to show the result to the user.

I suggest to change the function into this:

def is_isosceles(side1, side2, side3):
    return side1 == side2 or side1 == side3 or side2 == side3

#usage
print("This is an isosceles triangle!" if is_isosceles(side1, side2, side3)
    else "This is not an isosceles triangle!")
\$\endgroup\$
1
  • 1
    \$\begingroup\$ @Carcigenicate Added a couple of sentences. \$\endgroup\$ – Janne Karila Mar 30 '17 at 5:30
4
\$\begingroup\$

Don't mix up the presentation and logic

Many don't like the usage of time.sleep() in your code. I don't have a strong opinion about sleeping vs. not sleeping, but I think the real problem is a different one: any small change of the behavior of the code will take a disproportionate effort. For example, getting rid of the sleep() altogether or just reducing the sleeping intervals will require removing or rewriting large portions of the code.

The key idea how to avoid this is to try to reduce the dependencies between the user interface and the inner logic of the application. For a simple example like this, the preferable method is probably just to use as simple and minimalistic user interface and presentation as possible - so without any of the bells and whistles of sleep() and print(""). However, if you want to do things in a more scalable way, introduce a presentation abstraction - implement one or more different run-time behaviors, and provide the user with a single interface to select one or more of these behaviors. At the runtime, use this abstraction to invoke a specific behavior - decide which one exactly based on configuration settings, user input, etc. For example, there could be a "sleepy" behavior that internally calls time.sleep(), and there could be the default behavior that does nothing. The first will satisfy you, while the second - most of the rest.

Again, this is not crucial and probably an overkill for this simple task, but absolutely necessary when you'll want to scale up and develop more realistic applications.

Don't use magic numbers

What if you want to decrease sleep time two times everywhere for some reason? (Maybe some people like having shorter sleep intervals, but some longer.) Manually searching and replacing 2.5 and the rest of constants everywhere will be a pain.

Instead, define a named constant. It's too bad that the Python language does not have support for real constants - so often uppercase variables are used instead. For example, you could define these symbolic names and use them everywhere instead of directly writing in the number:

DELAY_SHORT_SEC = 0.25
DELAY_MEDIUM_SEC = 1.0
DELAY_LONG_SEC = 2.5

Example

The presentation abstraction could be a class like this:

class Presentation:
    DELAY_SHORT_SEC = 0.25
    DELAY_MEDIUM_SEC = 1.0
    DELAY_LONG_SEC = 2.5

    def __init__(self, do_sleep = False):
        self.do_sleep = do_sleep

    def delay(self, seconds):
        if self.do_sleep:
            time.sleep(seconds)

    def newline(self):
        print("")  # could be overriden

The usage of this would be like this - I added a new function to avoid potential duplication:

def ask_side_length(name, presentation):
    presentation.delay(Presentation.DELAY_SHORT_SEC)
    # ...
    # most of the rest is unchanged

def ask_all_side_lengths(presentation):
    presentation.delay(Presentation.DELAY_LONG_SEC)
    side1 = ask_side_length("first", presentation)
    side2 = ask_side_length("second", presentation)
    side3 = ask_side_length("third", presentation)
    presentation.newline()
    presentation.delay(Presentation.DELAY_MEDIUM_SEC)
    return side1, side2, side3

Finally, the usage:

default_presentation = Presentation()
sleepy_presentation = Presentation(do_sleep = True)
# does sleep
ask_all_side_lengths(sleepy_presentation)
# does not sleep
ask_all_side_lengths(default_presentation)
\$\endgroup\$

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.