7
\$\begingroup\$

I've been fiddling around with some easy code challenges and there's one about determining if a triangle is equilateral, isosceles, or scalene.

I've come up with a working solution, but I feel this could be greatly improved and/or simplified.

The sides of a triangle come in a list e.g. [2, 2, 2] or [0, 0, 0].

Here's what I've got.

def is_triangle(sides):
    if min(sides) <= 0:
        return False
    if sum(sorted(sides)[:-1]) < sorted(sides)[-1]:
        return False
    return True


def equilateral(sides):
    triangle = is_triangle(sides)
    if triangle:
        x, y, z = sides
        return x == y == z
    else:
        return False


def isosceles(sides):
    triangle = is_triangle(sides)
    if triangle:
        x, y, z = sides
        return x == y or y == z or z == x
    else:
        return False


def scalene(sides):
    if equilateral(sides) or isosceles(sides):
        return False
    return is_triangle(sides)

Also, I'm adding a simple unit test module.

import unittest

from triangle import equilateral, isosceles, scalene

class TestEquilateralTriangle(unittest.TestCase):
    def test_all_sides_are_equal(self):
        self.assertIs(equilateral([2, 2, 2]), True)

    def test_all_zero_sides_is_not_a_triangle(self):
        self.assertIs(equilateral([0, 0, 0]), False)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)

    def test_sides_may_be_floats(self):
        self.assertIs(equilateral([0.5, 0.5, 0.5]), True)


class TestIsoscelesTriangle(unittest.TestCase):
    def test_last_two_sides_are_equal(self):
        self.assertIs(isosceles([3, 4, 4]), True)

    def test_equilateral_triangles_are_also_isosceles(self):
        self.assertIs(isosceles([4, 4, 4]), True)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)

    def test_sides_may_be_floats(self):
        self.assertIs(isosceles([0.5, 0.4, 0.5]), True)


class TestScaleneTriangle(unittest.TestCase):
    def test_no_sides_are_equal(self):
        self.assertIs(scalene([5, 4, 6]), True)

    def test_all_sides_are_equal(self):
        self.assertIs(scalene([4, 4, 4]), False)

    def test_third_triangle_inequality_violation(self):
        self.assertIs(isosceles([3, 1, 1]), False)


if __name__ == "__main__":
    unittest.main()

I'd appreciate some feedback on my code.

\$\endgroup\$
18
\$\begingroup\$

A triangle has, by definition, three sides. I find it therefore weird to take a single sides argument, which could be of any size. This opens you up to obscure bugs, such as these ones, which are not covered in your tests:

>>> is_triangle([1,2,3,4])
True    # ?
>>> is_triangle([1, 1])
True    # ???
>>> is_triangle([float('nan')])
True    # WTF?

Instead, just explicitly take three arguments. The sides of a triangle are customarily called a, b, c.

def is_triangle(a, b, c):
    a, b, c = sorted([a, b, c])
    return a > 0 and a + b > c

This uses the fact that after the sorted, a is always the smallest side, as mentioned in the comments.

The only thing you need to change in your calling code is to call this with is_triangle(*sides), i.e. use tuple unpacking.


Your other functions can also be shortened a bit. Try to put multiple checks in one line to return right away (but don't push it if it gets too complicated).

def equilateral(a, b, c):
    return is_triangle(a, b, c) and a == b == c

Instead of manually checking all combinations of sides for equality, just use set to get rid of multiples:

def isosceles(a, b, c):
    return is_triangle(a, b, c) and len(set([a, b, c])) <= 2

def scalene(a, b, c):
    return is_triangle(a, b, c) and len(set([a, b, c])) == 3

Note that all functions need to use is_triangle. You could define a decorator that makes sure the input is a triangle:

from functools import wraps

def ensure_triangle(func):
    @wraps(func)
    def wrapper(a, b, c):
        return is_triangle(a, b, c) and func(a, b, c)
    return wrapper

@ensure_triangle
def equilateral(a, b, c):
    return a == b == c

@ensure_triangle
def scalene(a, b, c):
    return len(set([a, b, c])) == 3

@ensure_triangle
def isosceles(a, b, c):
    return len(set([a, b, c])) <= 2
\$\endgroup\$
  • 8
    \$\begingroup\$ One other change I would suggest is renaming those methods to start with is_ for consistency and clarity. I really like the use of a decorator here. \$\endgroup\$ – Will Da Silva Jul 25 at 18:58
6
\$\begingroup\$

After a very quick pass, here's what I have for you:

I assumed ... sorted(sides)[-1]: was a typo, so I removed the :.

  • Return Expressions: It's better to return expressions than to just else: return False. Returning the expression that is evaluated saves time, and looks cleaner.
  • Docstrings: I'm a stickler for docstrings. Even if the method is blatantly obvious about its function, you should still include a docstring. Keeps you in the practice for when you start writing bigger programs that require more detail.

Final Code

def is_triangle(sides):
    """ Determines if the list passed is a triangle """
    return False if min(sides) <= 0 or sum(sorted(sides)[:-1]) < sorted(sides)[-1] else True

def equilateral(sides):
    """ Determines if the list passed is an equilateral triangle """
    if is_triangle(sides):
        x, y, z = sides
        return x == y == z
    return False


def isosceles(sides):
    """ Determines if the list passed is an isosceles triangle """
    if is_triangle(sides):
        x, y, z = sides
        return x == y or y == z or z == x
    return False


def scalene(sides):
    """ Determines if the list passed is a scalene triangle """
    return False if equilateral(sides) or isosceles(sides) else is_triangle(sides)
\$\endgroup\$
  • 1
    \$\begingroup\$ I find your is_triangle() implementation awkward. It would be clearer to write return min(sides) > 0 and sum(sorted(sides)[:-1])) > sorted(sides)[-1] \$\endgroup\$ – 200_success Jul 25 at 22:35
  • 1
    \$\begingroup\$ The return False if ... ternary in scalene is missing an else. Also surely it would be easier to use not expr instead of the pattern False if expr else True \$\endgroup\$ – spyr03 Jul 26 at 14:08
2
\$\begingroup\$

Consider using doctest to keep the tests close to the code and more readable. For example:

def equilateral(sides):
    '''
    True if the 'sides' argument represents an equilateral triangle
    (all sides of equal length).

    >>> equilateral([2, 2, 2])
    True
    >>> equilateral([0, 0, 0])
    False
    >>> isosceles([3, 1, 1])
    False
    >>> equilateral([0.5, 0.5, 0.5])
    True
    '''
    x, y, z = sides
    return is_triangle(sides) and x == y == z

(er, why was that isosceles() test in TestEquilateralTriangle? Is that a copy-paste error?)

Then we can easily run all the tests:

if __name__ == "__main__":
    import doctest
    doctest.testmod()
\$\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.