9
\$\begingroup\$

I think I have pretty much the best code you can get when it comes to this particular task, but I'm always open to improvement. This code will check everything I can think of to make sure that it will actually work and if it is an isosceles triangle. It will make sure that none of the sides are 0, it will check whether the lengths of the sides are even possible and it will make sure that the user actually inputted a number.

import time

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)
while 2>1:
    try:
        side1 = float(input("How long is the first side of the triangle? "))
        if float(side1) == 0.0:
            print("This is an impossible triangle!")
            time.sleep(2.5)
            break
        else:
            0
    except ValueError:
        print("That's not a number...")
        time.sleep(2.5)
        break
    time.sleep(0.25)
    try:
        side2 = float(input("How long is the second side? "))
        if float(side2) == 0.0:
            print("This is an impossible triangle!")
            time.sleep(2.5)
            break
        else:
            0
    except ValueError:
        print("That's not a number...")
        time.sleep(2.5)
        break
    time.sleep(0.25)
    try:
        side3 = float(input("How long is the third side? "))
        if float(side3) == 0.0:
            print("This is an impossible triangle!")
            time.sleep(2.5)
            break
        else:
            0
    except ValueError:
        print("That's not a number...")
        time.sleep(2.5)
        break
    time.sleep(1)
    if side1 == side2 == side3:
        print("This is not an isosceles triangle!")
    elif float(side1)>float(side2) and float(side1)>float(side3):
        if (float(side2)+float(side3))<(float(side1)-0.000001):
            print("This is an impossible triangle!")
        else:
            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!")
    elif float(side2)>float(side1) and float(side2)>float(side3):
        if (float(side1)+float(side3))<(float(side2)-0.000001):
            print("This is an impossible triangle!")
        else:
            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!")
    elif float(side3)>float(side2) and float(side3)>float(side1):
        if (float(side1)+float(side2))<(float(side3)-0.000001):
            print("This is an impossible triangle!")
        else:
            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!")
    time.sleep(2.5)
    break
\$\endgroup\$
  • 2
    \$\begingroup\$ Well, I would not say your reviewers that you have the best code :) Wait, have you done that intentionally? \$\endgroup\$ – alecxe Mar 28 '17 at 19:31
  • \$\begingroup\$ If you have already created the best code for the task, why have you come here? \$\endgroup\$ – SaggingRufus Mar 29 '17 at 10:53
  • \$\begingroup\$ while 2>1: why? \$\endgroup\$ – njzk2 Mar 29 '17 at 15:38
  • \$\begingroup\$ if side1 == side2 == side3: print("This is not an isosceles triangle!") actually, this is subject to debate. \$\endgroup\$ – njzk2 Mar 29 '17 at 15:40
  • \$\begingroup\$ @njzk2 Take a look at the follow up question. \$\endgroup\$ – Gameskiller01 Mar 29 '17 at 16:31
6
\$\begingroup\$

The major thing that stood out to me was the sheer amount of repetition in your code. This will be the main concern of my review.

Your code isn't modular; it's just a long script of calculations and printing. What if you ever wanted to do the calculations based off of data from a file? Or the internet? Create a function that calculates whether or not a given triangle is isosceles, pass the data to it, and have it return True/False. I'm of the opinion that the more functions you have, the better (within reason). It gives you more small bits of code that can be reused in other places.

One major improvement you can make that will instantly neaten up your code is changing the way you take input from the user. When asking for input, you write out a full try/except for each side, even though they're really all the same. That can be reduced down to a single function that "safely" asks the user for input:

def ask_side_length (side_message):
    while True:
        try:
            length = float(input("How long is the " + side_message + " side of the triangle?"))

            if length <= 0.0:
                print("This is an impossible triangle!")

            else:
                return length

        except ValueError:
                print("That's not a number...")

This could be generalized, but for the sake of the review, I'm going to leave it like this. You could, for practice, create a function that accepts general input from a user, validates it, and loops when validation fails. Then ask_side_length could be defined in terms of that function.

A few changes I made:

  • The code is now a function. That means you now have a reusable bit of code you can use anywhere you want, without needing to retype it or copy and paste.

  • I'm using the True constant, since a Boolean comparison to achieve the same seems convoluted. This isn't code-golf! Shorter code does not necessarily mean better code.

  • I'm looping while this single request for input is bad. Previously, your entire program would stop if any input was bad. Bad input happens! Just ask again.

  • I excluded negative inputs as well, since I don't think a negative side-length makes sense in most contexts.

Now you can just call this function 3 times:

side1 = ask_side_length("first")
side2 = ask_side_length("second")
side3 = ask_side_length("third")

Practice turning repetitious code into a function. Really, that cannot be stressed enough. This will save you and your readers from tears.

\$\endgroup\$
11
\$\begingroup\$

It's a little hard to approach your code because you have a large amount of repetition. Modularize and reuse individual parts.


Example framework

One of the other answers goes a long way to improving it with a Triangle class, but I suspect that that's a little farther ahead of where you are right now. Here's an easy framework to consider that's essentially modular.

def prompt_float():
    """Prompt the user for a float until they provide a valid one. Return that float."""

def is_valid_side(side):
    """side is a float representing the side of a triangle. Return True iff it is > 0."""

def has_isosceles_relationship(sides):
    """
    sides is a list of 3 floats representing the sides of a triangle. Return True iff
    exactly two are equal.
    """

    # Hint: use a set here rather than many nearly identical comparisons

def is_possible_triangle(sides):
    """
    sides is a list of 3 floats representing the sides of an isosceles triangle. Return
    True iff the triangle is possible. A possible triangle is defined as one whose two
    equal sides are at least the length of its third side.
    """

    # Hint: sort sides and determine which side is unique, then do one comparison

def evaluate_isosceles_triangle(sides):
    """
    sides is a list of 3 floats representing the sides of a triangle. Return True iff
    that triangle is an isosceles triangle. (Equilateral should fail.)
    """

    # Validate individual sides
    for side in sides:
        if not is_valid_side(side):
            return False

    # Validate basic isosceles relationship
    if not has_isosceles_relationship(sides):
        return False

    # Validate that the triangle's dimensions are possible
    if not is_possible_triangle(sides):
        return False

    # Otherwise we are good
    return True


# Execute when this module is run directly
if __name__ == "__main__":

    # Prompt for three sides
    sides = []
    for _ in range(3):
       sides.append(prompt_float())

    # Evaluate and print result
    result = evaluate_isosceles_triangle(sides)
    if result:
        print("That is an isosceles triangle!")
    else:
        print("That is not an isosceles triangle.")

As has also been said, you can dispense with the time.sleep() calls and empty else blocks.


Doctesting

At some point you should also write unit tests for these functions. I think it's a bit much at once to put them in here, but for now you could consider at least adding simpler doctests to be sure the functions work the way they should with sample inputs. Here's an example:

def has_isosceles_relationship(sides):
    """
    sides is a list of 3 floats representing the sides of a triangle. Return True iff
    exactly two are equal.

    >>> has_isosceles_relationship([1, 2, 3])
    False
    >>> has_isosceles_relationship([2, 2, 3])
    True
    >>> has_isosceles_relationship([3, 2, 3])
    True
    >>> has_isosceles_relationship([2, 2, 2])
    False
    """

Here we test four cases: all 3 sides different; 2/3 sides equal; 2/3 sides equal, not adjacent; 3/3 equal. You can run these docstring examples automatically by including this code at the bottom of a file:

if __name__ == "__main__":
    import doctest
    doctest.testmod()

Why modular?

A word on modularity and why this is being proposed to you by several people. There are several benefits to breaking code into smaller parts. Among them:

  1. Less repetition. By giving a block of lines a name, i.e. making it a function, you can call it more than once without repeating those lines. (In your code, there are actually stronger ways to remove repetition, but it still helps.)

  2. More organization. By breaking up and naming the various functions your code has, you get a better sense of how your program works. This also helps people understand your code (including yourself three months from now!), and that helps maintain it.

  3. Easier documentation and testing. Again, by isolating functions, you can target them directly with more detailed comments and more precise tests. This also means that if something breaks, you know what broke and where.

  4. Clearer abstraction. Think about whether your code does anything more than once, and extract common parts. For example, you have three blocks prompting for sides. This is really just one kind of thing — prompting for a float. I didn't do it above, but you could maintain the distinct nature of these blocks via parametrization: add a message argument to the prompt_float function that gets printed with input, so that you can still have different behaviour each time.

These are all interconnected and there are other ways to see the benefits. But the key thing is that you give yourself the power to take your program apart, reconstruct it, analyze it, change your mind about individual things, and more. One of the other answers says that modularity would be "too much abstraction" for an easy problem like this. But you have to get your head into the abstraction game early, especially when you're dealing with small, manageable chunks like this, as practice for the bigger stuff. Remember, technical debt builds up, and sooner or later you pay for any lazy programming choices — with interest!

\$\endgroup\$
  • \$\begingroup\$ Very nice! The only change I would suggest is change evaluate_isosceles_triangle to accept a list of sides, add if __name__ == '__main__' as suggested by @alecxe, and do the prompting for sides outside of evaluate_isosceles_triangle. That way evaluate_isosceles_triangle can be imported and called from elsewhere. \$\endgroup\$ – PurpleDiane Mar 28 '17 at 20:36
  • \$\begingroup\$ It's now "one of the below answers" :) \$\endgroup\$ – alecxe Mar 28 '17 at 22:40
  • \$\begingroup\$ Good call! Incorporated the edits. \$\endgroup\$ – Luke Sawczak Mar 29 '17 at 2:07
  • \$\begingroup\$ I would use _ as an unused iteration variable and do sides = [prompt_float() for _ in range(3)]. \$\endgroup\$ – Graipher Mar 29 '17 at 9:11
  • \$\begingroup\$ I'd recommend also showing how to perform some simple unit tests, starting with (0,0,0), (0,0,1), (0,1,1) and onwards until the cases are all covered. Another hint: it helps if you sort the sides before making any comparisons. \$\endgroup\$ – Toby Speight Mar 29 '17 at 9:44
4
\$\begingroup\$

The code is very far from being "the best you can get":

  • large amount of code duplication, follow the DRY principle
  • you can replace while 2>1 with a simple while True, though since you break the loop at the end, I don't see a point of using the endless loop at all (unless you've added this break for debugging purposes)
  • the else part is optional, you don't need these else: 0 blocks
  • the main program logic needs to be put into if __name__ == '__main__' so that it would not be executed if the module imported
  • you can use a multi-line string to define your initial greeting message

A more modular, readable and concise approach

What if we define a Triangle class with handy isosceles and equilateral properties, putting the side validation logic into the class constructor. We'll also read the triangle sides in a loop. Something along these lines:

import time


class Triangle:
    def __init__(self, sides):
        self.side1, self.side2, self.side3 = sides

        if any(side == 0 for side in sides):
            raise ValueError("This is an impossible triangle!")

        if (self.side2 + self.side3) < (self.side1 - 0.000001):  # TODO: check other sides
            raise ValueError("This is an impossible triangle!")

    @property
    def isosceles(self):
        if self.side1 == self.side2 or self.side2 == self.side3 or self.side2 == self.side3:
            return True
        return False

    @property
    def equilateral(self):
        return self.side1 == self.side2 == self.side3


if __name__ == '__main__':
    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)

    sides = []
    for number in range(3):
        try:
            side = float(input("How long is the {number} side of the triangle? ".format(number=number+1)))
            sides.append(side)
        except ValueError:
            print("That's not a number...")
            raise

    triangle = Triangle(sides)
    if triangle.isosceles:
        print("This is an isosceles triangle!")
    else:
        print("This is not an isosceles triangle!")

It is, of course, not the best Triangle class you may have - for instance, there is probably no need to have sideN instance variables and we can just get away with sides only. But, hope this would be a good start for you.

\$\endgroup\$
  • 2
    \$\begingroup\$ I'd check the sides with if (sides[0] in [sides[1], sides[2]]) or (sides[1] == sides[2]): \$\endgroup\$ – ChatterOne Mar 28 '17 at 21:05
  • \$\begingroup\$ why have you not replaced if (self.side2 + self.side3) < (self.side1 - 0.000001) with if (self.side2 + self.side3) <= (self.side1) \$\endgroup\$ – Destructible Lemon Mar 29 '17 at 0:52
3
\$\begingroup\$

You can also vastly simplify the check for a valid triangle by (mis) using Heron's Formula.

Evaluate:

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

If areaSquared is zero, the triangle is degenerate, meaning all three points are co-linear; if areaSquared is negative, it's not a valid triangle.

This can be further optimized. The initial semiSum * can be elided, since it isn't needed to solve the specific problem outlined by the OP.

\$\endgroup\$
  • 1
    \$\begingroup\$ Using semiSum = (side1 + side2 + side3) / 2.0 areaSquared = semiSum * (semiSum - side1) * (semiSum - side2) * (semiSum - side2) 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() says that a triangle with the sides 64, 64, and 643 is possible. Is there something I'm doing wrong? \$\endgroup\$ – Gameskiller01 Mar 28 '17 at 21:26
  • 1
    \$\begingroup\$ @Gameskiller01 you did nothing wrong, you're a victim of my mistake. I had a typo, now corrected, where the last term in the areaSquared computation was (semiSum - side2) when it should have been (semiSum - side3). If you try it like that it should work properly. \$\endgroup\$ – dgnuff Mar 29 '17 at 6:17
  • \$\begingroup\$ I honestly don't think that your suggestion of 3 additions, 1 division, 3 subtractions, 3 multiplications and one comparison is simpler than 3 additions and 3 comparisons (you just need to check that the sum of two sides is at least as big as the third). \$\endgroup\$ – ChatterOne Mar 29 '17 at 6:52
1
\$\begingroup\$

This is wrong:

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

By definition, every equilateral triangle is also an isosceles triangle:

An equilateral triangle is therefore a special case of an isosceles triangle having not just two, but all three sides and angles equal.

\$\endgroup\$
  • 3
    \$\begingroup\$ "Sometimes it is specified as having two and only two sides of equal length, and sometimes as having at least two sides of equal length ... Euclid defined an isosceles triangle as one having exactly two equal sides, but modern treatments prefer to define them as having at least two equal sides" (Wikipedia) \$\endgroup\$ – Luke Sawczak Mar 29 '17 at 2:15
-1
\$\begingroup\$

It's a simple problem. First consider if the 3 sides can form triangle. If so, consider what type of triangle it is. Do induction with the conclusion, and report whether or not it is isoscele.

For only 3 sides, more abstraction would be overkill. Coding is faster with simple methods, and maintainers would read your codes faster. For 30 sides, definitely do.

# -- isIsosceles.py

def printError():
    print("This is not a number, try again.")
def is_number(s):
    try:
        float(s)
        return True
    except ValueError:
        if s == "FirstRun":
            pass
        else:
            printError()
        return False

def w(a):
    return a-1


def isTriangle(sides):
    HalfSidesLengthSum = 0.5*sum(sides)
    for side in sides:
        if side >= HalfSidesLengthSum:
            return False
    return True

def isEquilateral(sides):
    if sides[w(1)] == sides[w(2)]:
        if sides[w(2)] == sides[w(3)]:
            return True
    return False
def isIsosceles(sides):
    if sides[w(1)] == sides[w(2)]:
        return True
    elif sides[w(2)] == sides[w(3)]:
        return True
    elif sides[w(1)] == sides[w(3)]:
        return True
    else:
        return False
def WhatsThis(sides):
    if isTriangle(sides):
        if isEquilateral(sides):
            return ("This is equilateral, and, a fortiori, isosceles.")
        else:
            if isIsosceles(sides):
                return ("This is isosceles.")
            else:
                return ("This is not isosceles.")
    return ("The sides cannot form triangle, and, a fortiori, this is not isosceles.")


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.")
side1="FirstRun"
side2="FirstRun"
side3="FirstRun"

while is_number(side1) == False:
    side1 = input("How long is the first side of the triangle? ")
while is_number(side2) == False:
    side2 = input("How long is the second side? ")
while is_number(side3) == False:
    side3 = input("How long is the third side? ")

sides=[float(side1),float(side2),float(side3)]
Verdict = WhatsThis(sides)
print(Verdict)
\$\endgroup\$
  • \$\begingroup\$ We don't normally test for equality to False (or True). Instead, we use the not operator: while not is_number(side1): \$\endgroup\$ – Toby Speight Mar 29 '17 at 9:40
  • \$\begingroup\$ @Toby Speight This is a Pythonic feature. If you work on different languages, using concepts working on most languages is less error-prone. If you need to come back to the project after a few months, looking up online for "How to do XYZ in Python" can waste a lot of time. \$\endgroup\$ – zyc Mar 29 '17 at 17:47
  • \$\begingroup\$ @Toby Speight The concept of inverter can be confusing, especially with not is_not_number(side1). The main problem is that if you later want to change the line to compare 2 valules, such as while f(x) != arr[0], you will have to change the syntax. \$\endgroup\$ – zyc Mar 29 '17 at 19:15

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.