7
\$\begingroup\$

I wrote some code to estimate the solution to the following problem:

If you break a line segment at two random points, what is the probability that the new line segments can form a triangle?

The code is relatively simple, and it works. However, I can't shake the feeling that each of the three functions I've created could be written more "Pythonically." For example, the first function (can_form_triangle), explicitly writes out the combinations of pairs from (a, b, c) and then compares them to the element of (a, b, c) which is not in the pair. There must be a way to express this more succinctly, more elegantly — more Pythonically.

To be clear, I'd just like advice on refactoring the code to be more elegant/Pythonic. I am not looking for advice on how to change the actual functionality to solve the problem more efficiently.

Here's the code:

#!/usr/bin/python3

from random import random


def can_form_triangle(a, b, c):
    '''Determines if lengths a, b, and c can form a triangle.

    Args:
        a, b, c: Number representing the length of a side of a (potential) triangle

    Returns:
        True if all pairs from (a, b, c) sum to greater than the third element
        False otherwise
    '''
    ab = a + b
    ac = a + c
    bc = b + c

    if (ab > c and ac > b and bc > a):
        return True

    return False


def try_one_triangle():
    '''Simulates breaking a line segment at two random points and checks if the segments can form a triangle.

    Returns:
        True if the line segments formed by breaking a bigger line segment at two points can form a triangle
        False otherwise
    '''
    first_point = random()
    second_point = random()
    sorted_points = sorted((first_point, second_point))

    return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])


def estimate_triangle_probability():
    num_success = num_attempts = 0
    for _ in range(10000000):
        num_success += 1 if try_one_triangle() else 0
        num_attempts += 1

    print('Success:', num_success)
    print('Attempts:', num_attempts)
    print('Ratio:', num_success / (num_attempts))


if __name__ == '__main__':
    estimate_triangle_probability()
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! In your can_form_triangle() you can directly return ab > c and ac > b and bc > a \$\endgroup\$ – Grajdeanu Alex. May 25 at 16:01
  • 1
    \$\begingroup\$ @GrajdeanuAlex. you might want to add some more and make that an answer. \$\endgroup\$ – pacmaninbw May 25 at 17:39
  • \$\begingroup\$ Actually return ab > c or ac > b or bc > a \$\endgroup\$ – Joop Eggen May 26 at 5:41
9
\$\begingroup\$

Direct boolean return

As @Grajdeanu Alex says, this:

if (ab > c and ac > b and bc > a):
    return True

return False

can simply be

return ab > c and ac > b and bc > a

Type hints

def can_form_triangle(a, b, c):

can be

def can_form_triangle(a: float, b: float, c: float) -> bool:

Sort unpack

first_point = random()
second_point = random()
sorted_points = sorted((first_point, second_point))

return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])

can be

    first_point, second_point = sorted((random(), random()))

    return can_form_triangle(first_point, second_point - first_point, 1 - second_point)

Digit triples

10000000 is more easily read as 10_000_000.

Attempt looping

num_attempts will evaluate to 10_000_000 so it's not worth tracking unless you add an early-exit mechanism.

The whole loop can be replaced with

num_success = sum(
    1
    for _ in range(10_000_000)
    if try_one_triangle()
)

Redundant parentheses

print('Ratio:', num_success / (num_attempts))

does not need the inner parens.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Exactly what I was looking for! Thank you so much. I didn't know you could write 10000000 as 10_000_000. That's pretty cool. \$\endgroup\$ – Josh Clark May 25 at 16:52
  • \$\begingroup\$ @JoshClark: It's Python 3.6+. \$\endgroup\$ – Graipher May 25 at 17:31
1
\$\begingroup\$

Quite nicely done. However there is always room for improvement. In order of severity

Looping

You do

def estimate_triangle_probability():
    num_success = num_attempts = 0
    for _ in range(10000000):
        num_success += 1 if try_one_triangle() else 0
        num_attempts += 1

If you were to need the loop counter inside the loop you would rather do

def estimate_triangle_probability():
    num_success = 0
    for num_attempts in range(10000000):
        num_success += 1 if try_one_triangle() else 0

As you do not need the counter inside the loop but using the value after the loop as 'number of runs' you shall do

def estimate_triangle_probability():
    num_success = 0
    num_attempts = 10000000
    for _ in range(num_attempts):
        num_success += 1 if try_one_triangle() else 0

Looping with extra counters is very error prone. you should really avoid that.

As we already touch this code we also introduce num_attempts as parameter which gives nice testabilty

def estimate_triangle_probability(num_attempts):
    num_success = 0
    for _ in range(num_attempts):
        num_success += 1 if try_one_triangle() else 0

Another minor readability improvement is the ternary if

num_success = 0
for _ in range(num_attempts):
    num_success += 1 if try_one_triangle() else 0

which in this case is imho more readable in the form

num_success = 0
for _ in range(num_attempts):
    if try_one_triangle():
        num_success += 1

Differently we can eliminate the explicit loop for a comprehension

num_success = sum(1 for _ in range(num_attempts) if try_one_triangle())

So we get rid of the counter initialization and increment.

Return boolean expressions

You do

if (ab > c and ac > b and bc > a):
    return True

return False

which is an 'anti-pattern'. Instead do

return ab > c and ac > b and bc > a

Temporary variables

You do

ab = a + b
ac = a + c
bc = b + c

if (ab > c and ac > b and bc > a):
    # ...

which has no better readability or documentation. There is nothing wrong with

if a + b > c and a + c > b and b + c > a:
    # ...

The same goes for

first_point = random()
second_point = random()
sorted_points = sorted((first_point, second_point))
return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])

which may read

sorted_points = sorted(random(), random())
return can_form_triangle(sorted_points[0], sorted_points[1] - sorted_points[0], 1 - sorted_points[1])

There is good reason for temporaries if the name of the temporary serves documentation. Here the names do not add value.

To improve readability of the return expression we do

x, y = sorted((random(), random()))
return can_form_triangle(x, y-x, 1-y)

Overengineered

After removing unnecessary temporaries the remaining code looks like

#!/usr/bin/python3

from random import random


def can_form_triangle(a, b, c):
    """Determines if lengths a, b, and c can form a triangle.

    Args:
        a, b, c: Number representing the length of a side of a (potential) triangle

    Returns:
        True if all pairs from (a, b, c) sum to greater than the third element
        False otherwise
    """

    return a + b > c and a + c > b and b + c > a


def try_one_triangle():
    """Simulates breaking a line segment at two random points and checks if the segments can form a triangle.

    Returns:
        True if the line segments formed by breaking a bigger line segment at two points can form a triangle
        False otherwise
    """

    x, y = sorted((random(), random()))
    return can_form_triangle(x, y-x, 1-y)


def estimate_triangle_probability(num_attempts):
    num_success = sum(1 for _ in range(num_attempts) if try_one_triangle())

    print('Success:', num_success)
    print('Attempts:', num_attempts)
    print('Ratio:', num_success / num_attempts)


if __name__ == '__main__':
    estimate_triangle_probability(10000000)

We notice that most functions have a single line of code. We have docstrings for two one-line helpers but none for the top level estimate_triangle_probability(num_attempts). If we eliminate the two helpers we get

#!/usr/bin/python3

from random import random


def estimate_triangle_probability(num_attempts):
    num_success = 0
    for _ in range(num_attempts):
        # break a line of length 1 two times to get three segments
        x, y = sorted((random(), random()))
        # segments can form a triangle if all are shorter than half the perimeter
        if all(s < 0.5 for s in (x, y-x, 1-y)):
            num_success += 1
    return num_success / num_attempts


if __name__ == '__main__':
    num_attempts = 10000000
    ratio = estimate_triangle_probability(num_attempts)
    print('Attempts:', num_attempts)
    print('Ratio:', ratio)

Here we also moved output code from the remaining function to main and introduced a return value instead. The final result is is maybe a little too dense for a programming course. Left to do: We could still improve some names. Also the remaining function needs documentation.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice answer. In the last code, I would not even do the division,but let the caller worry about that, and just return num_success or a tuple num_success, num_attempts. You can also use the fact that a True counts as 1 when added to an int to rewrite the for-loop to for _ in range(num_attempts):; x, y = sorted((random(), random())); num_success += all(s < 0.5 for s in (x, y-x, 1-y)) \$\endgroup\$ – Maarten Fabré May 29 at 7:03
  • \$\begingroup\$ @MaartenFabré: Division - I did the division to fit the name and I actually prefer it like that. \$\endgroup\$ – stefan May 29 at 12:22

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.