Perimeter of a polygon given its vertices

I recently completed this code challenge so just looking for some feedback on my code. The task is to read polygon coordinates from a file and calculate the perimeter, I was provided with a script containing bugs and asked to fix and improve the code.

Example input

data.csv

x,y
-16,-18
-18,-10
2,6
1,-2
9,-13
-7,-14
-9.8,18
-11,15
11.9,3
7,-4

Provided script

perimeter.py

import csv

def main(file_name):

    fp = open(file_name)
    reader = csv.reader(fp)

    points = []
    for row in reader:
        x = row[0]
        y = row[1]
        points.append((x,y))

    length = perimiter(points)

    print length

if __name__ == "__main__":

    file_name = sys.argv[0]
    main(file_name)

def perimiter(points):
    """ returns the length of the perimiter of some shape defined by a list of points """
    distances = get_distances(points)

    length = 0
    for distance in distances:
        length = length + distance

    return length

def get_distances(points):
    """ convert a list of points into a list of distances """
    i = 0
    distances = []
    for i in range(len(points)):
        point = points[i]
        next_point = points[i+1]
        x0 = point[0]
        y0 = point[1]
        x1 = next_point[1]
        y1 = next_point[1]

        point_distance = get_distance(x0, y0, x1, y1)
        distances.append(point_distance)

def get_distance(x0, y1, x1, y1):
    """ use pythagorean theorm to find distance between 2 points """
    a = x1 - x2
    b = y1 - y2
    c_2 = a*a + b*b

    return c_2 ** (1/2)

My Solution

perimeter.py

import sys
from csv_points_reader import CsvPointsReader
from perimeter_calculator import PerimeterCalculator

def main(file_name):
    reader = CsvPointsReader()
    calculator = PerimeterCalculator()

    points = reader.get_points(file_name)
    length = calculator.get_perimeter(points)

    print length

if __name__ == "__main__":
    file_name = sys.argv[1]
    main(file_name)

csv_points_reader.py

`

Off the bat, I don't like having your class arrangement. This isn't java, Python is pretty loose about how you want to organise. In particular, they're just classes that are used to call functions. What's the point of that? You could rewrite these to be modules instead. Modules can hold a set of functions that can be called on from scripts they're imported in. For instance:

import csv_points_reader as reader

points = reader.get_points(file_name)

Those lines essentailly work the same, except without redundant instantiation of a class instance. Module functions can call on each other without using self because a module is already its own namespace.

Point is a pretty good class, but you could also just use a named tuple. In fact, a point is the example the docs use to demonstration namedtuples. So instead of your class definition you just need:

from collections import namedtuple

Point = namedtuple('Point', ['x', 'y'])

I find it a little weird that invalid co-ordinates are flat out ignored, this may lead to masking errors and confusing results. Especially as your whole script is a black box that only ever prints a number. It may be worth at least logging invalid co-ordinates for the sake of reference.

Both _is_coordinate and _is_float are pretty limited and one is only ever used by the other. I'd fold them into one function. I'd also point out that you only ever need the first two rows, but you're validating all of them. That's really not necessary, you could just reduce it to this:

def _is_coordinate(self, row):
    """ check if row contains a valid coordinate """

    try:
        float(row[0])
        float(row[1])
        return True
    except ValueError:
        return False

get_perimeter can be massively simplified just by using sum, which will get the sum of all values in a list:

def get_perimeter(self, points):
    """ returns the length of the perimiter of some shape defined by a list of points """

    return sum(get_distances(points))

It's debatable whether or not you even need this function, I'd almost just call sum from your main script.

get_distances could be rewritten as a generator. Generators basically use yield (similar to return) to yield one value at a time. They're used in iterations, or functions that run iterations (like sum does). They're more performant than returning whole lists. And if you ever need the list you can just call list(get_distances(points)). Here's the small changes you could make:

def get_distances(self, points):
    """ convert a list of points into a list of distances """

    circular_buffer = cycle(points)
    previous_point  = circular_buffer.next()

    for i in range(len(points)):
        point = circular_buffer.next()
        yield get_distance(previous_point, point)
        previous_point = point

Each time you find a value from get_distance you're yielding it, rather than building up a full list to return.

StackExchange Code Review Q#120141, answer score: 2