# 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)

points = []
x = row
y = row
points.append((x,y))

length = perimiter(points)

print length

if __name__ == "__main__":

file_name = sys.argv
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
y0 = point
x1 = next_point
y1 = next_point

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 perimeter_calculator import PerimeterCalculator

def main(file_name):
calculator = PerimeterCalculator()

length = calculator.get_perimeter(points)

print length

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


import csv
from point import Point

""" read list of coordinates from csv file """
def __init__(self):

def get_points(self, file_name):
""" get list of type Point from csv file """
fp = open(file_name)

points = []
if self._is_coordinate(row):
x = float(row)
y = float(row)
points.append(Point(x,y))

return points

def _is_coordinate(self, row):
""" check if row contains a valid coordinate """
return all(self._is_float(item) for item in row)

def _is_float(self, str):
""" check if string is a float"""
try:
float(str)
return True
except ValueError:
return False


perimeter_calculator.py

import math
from itertools import cycle
from point import Point

class PerimeterCalculator(object):
""" calculate perimeter of a polygon """
def __init__(self):
super(PerimeterCalculator, self).__init__()

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

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

return length

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

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

for i in range(len(points)):
point = circular_buffer.next()

distance = self.get_distance(previous_point, point)
distances.append(distance)

previous_point = point

return distances

def get_distance(self, point1, point2):
""" use pythagorean theorm to find distance between 2 points """
a = point1.x - point2.x
b = point1.y - point2.y
return math.sqrt(a**2 + b**2)


point.py

class Point(object):
def __init__(self, x, y):
super(Point, self).__init__()
self.x = x
self.y = y


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



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)
float(row)
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.

• @Glad to help, and thanks for accepting my answer! But you may want to wait a bit for other people before you accept. Sometimes answers on Code Review take a while, and people can be dissuaded from reviewing when they seen an accepted answer. I generally recommend waiting at least a day. You can unaccept my answer if you wish, the decision is up to you. – SuperBiasedMan Feb 16 '16 at 11:29