5
\$\begingroup\$

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

import csv
from point import Point

class CsvPointsReader(object):
    """ read list of coordinates from csv file """
    def __init__(self):
        super(CsvPointsReader, self).__init__()

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

        points = []
        for row in reader:
            if self._is_coordinate(row):
                x = float(row[0])
                y = float(row[1])
                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
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ @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. \$\endgroup\$ – SuperBiasedMan Feb 16 '16 at 11:29

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.