CODEWITHSUNDEEP
pythonmathpolygon
Ogbuefi Akana
Ogbuefi Akana

Reputation: 11

Tips on improving this function?

This may be quite a green question, but I hope you understand – just started on python and trying to improve. Anyways, wrote a little function to do the "Shoelace Method" of finding the area of a polygon in a Cartesian plane (see this for a refresher).

I want to know how can I improve my method, so I can try out fancy new ways of doing the same old things.

    def shoelace(list):
        r_p     = 0         # Positive Values
        r_n     = 0         # Negative Values

        x, y    = [i[0] for i in list], [i[1] for i in list]
        x.append(x[0]), y.append(y[0])

        print(x, y)

        for i in range(len(x)):
            if (i+1) < len(x):
                r_p += (x[i] * y[i+1])
                r_n += (x[i+1] * y[i])
            else:
                break

        return ((abs(r_p - r_n))/2)

Upvotes: 1

Views: 155

Answers (2)

Karl Knechtel
Karl Knechtel

Reputation: 61525

  • Don't use short variable names that need to be commented; use names that indicate the function.

  • list is the name of the built-in list type, so while Python will let you replace that name, it's a bad idea stylistically.

  • , should not be used to separate what are supposed to be statements. You can use ;, but it's generally better to just put things on separate lines. In your case, it happens to work because you are using .append for the side effect, but basically what you are doing is constructing the 2-tuple (None, None) (the return values from .append) and throwing it away.

  • Use built-in functions where possible for standard list transformations. See the documentation for zip, for example. Except you don't really need to perform this transformation; you want to consider pairs of adjacent points, so do that - and take apart their coordinates inside the loop.

  • However, you can use zip to transform the list of points into a list of pairs-of-adjacent-points :) which lets you write a much cleaner loop. The idea is simple: first, we make a list of all the "next" points relative to the originals, and then we zip the two point-lists together.

  • return is not a function, so the thing you're returning does not need surrounding parentheses.

  • Instead of tallying up separate positive and negative values, perform signed arithmetic on a single value.

def shoelace(points):
    signed_double_area = 0

    next_points = points[1:] + points[:1]

    for begin, end in zip(points, next_points):
        begin_x, begin_y = begin
        end_x, end_y = end
        signed_double_area += begin_x * end_y
        signed_double_area -= end_x * begin_y

    return abs(signed_double_area) / 2

Upvotes: 2

HerrKaputt
HerrKaputt

Reputation: 2604

Functionally, your program is quite good. One minor remark is to replace range(len(x)) with xrange(len(x)). It makes the program slightly more efficient. Generally, you should use range only in cases where you actually need the full list of values it creates. If all you need is to loop over those values, use xrange.

Also, you don't need the parenthesis in the return statement, nor in the r_p += and r_n += statements.

Regarding style, in Python variable assignments shouldn't be done like you did, but rather with a single space on each side of the = symbol:

r_p = 0
r_n = 0

Upvotes: 0

Related Questions