HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Right triangle calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
trianglecalculatorright

Problem

I'm trying to code Python in an object-oriented style, but it just seems very forced. Can anyone give me tips on this exercise of calculating the third leg of a right triangle from the other two?

I'm not looking for input santitization help! I'm aware the project doesn't check if the input is a positive integer (or if the hypotenuse is the longest side, etc), but would much rather improve the general flow of how an OO project should look than the security/sanity checks on user input.

```
#!/usr/bin/python

import math

#Triangle has three sides; two can be defined and the third is calculated
class Triangle:

def __init__(self):
self.side={"adjacent":0,"opposite":0,"hypotenuse":0}

def define_sides(self):
for i in self.side:
self.side[i]=self.get_side(i)

def print_sides(self):
for i in self.side:
print "side",i,"equals",self.side[i]

#return user integer or None if they enter nothing
def get_side(self,one_side):
prompt = "Enter length of "+one_side+": "
try:
return input(prompt)
except SyntaxError:
return None

def count_nones(self):
nones=0
for side, length in self.side.iteritems():
if self.side[side] == None:
nones+=1
return nones

def validate_input(self):
nNones=self.count_nones()

if nNones 1:
print "Only one side can be left blank."

def calculate_missing_length(self):
h=self.side["hypotenuse"]
a=self.side["adjacent"]
o=self.side["opposite"]

#hypotenuse is missing
if h == None:
self.side["hypotenuse"] = math.sqrt(aa+oo)

#adjacent is missing
if a == None:
self.side["adjacent"] = math.sqrt(hh-oo)

#opposite is missing
if o == None:
self.side["opposite"] = math.sqrt(hh-aa)

#begin program
triangle=Triangle()
triangle.define_sides()
triangle

Solution

State should always be valid

One of the main issues I see in your class design is that the state of your object is undefined most of the time. If I have a triangle, I want it to be... you know, a triangle. But when you start with:

triangle=Triangle()


You have an object with 3 zero sides. That's a degenerate object. Rather, you'll want to make it so that once you have a Triangle, then it must be valid.

Building up our Triangle

To that end, let's consider get_side(). As a member function, this makes little sense - it doesn't interact with Triangle at all. The get_* naming suggests that it's yielding some sort of member, when actually it's prompting the user for input. So let's rework it as a free function:

def input_side(side_name):
    try:
        return float(raw_input('Enter length of {}:'.format(side_name)))
    except ValueError:
        return None


Catching SyntaxError is questionable - it'd be better to use raw_input and a cast that returns a specific error. Otherwise, what if we had a syntax error? You'd hide it from yourself.

Once we have that, we can use that to call into our Triangle constructor:

triangle = Triangle(
    input_side('adjacent'),
    input_side('opposite'),
    input_side('hypotenuse')
)


where we rewrite the constructor to be:

def __init__(self, adjacent, opposite, hypotenuse):
    self.adjacent = adjacent
    self.opposite = opposite
    self.hypotenuse = hypotenuse


It is much better to simply have your members as members than to hide than in a dictionary. self.hypotenuse is much more direct than self.side['hypotenuse']!

Input Validation

Comparison against None should be done with is, not ==. And if you're iterating over a dictionary with iteritems(), you don't need lookup:

def count_nones(self):
    nones=0
    for side, length in self.side.iteritems():
        if length is None:
            nones+=1
    return nones


though the above can be abridged into:

def count_nones(self):
    return sum(1 for (side, length) in self.side.iteritems()
        if length is None)


Or, now that I'm proposing we not use a dictionary:

def count_nones(self):
    return (self.adjacent is None +
        self.opposite is None + 
        self.hypotenuse is None)


But really, you should do all of this external to your Triangle class. As in:

  • Input your sides



  • Make sure exactly 2 of them are valid



  • Calculate the third



  • Then construct your Triangle with all three sides



Printing

In Python, you would typically add a __str__ method instead of a print sort of method:

def __str__(self):
    return 'Triangle(adjacent={}, opposite={}, hypotenuse={})'.format(
        self.adjacent, self.opposite, self.hypotenuse
    )


namedtuple

If you follow where I'm going up to now - you'll see I've made Triangle a great deal simpler. It doesn't deal with inputs. It doesn't deal with calculations. It's just a triangle. And for simple classes like that, Python has namedtuple:

>>> Triangle = namedtuple('Triangle', 'opposite adjacent hypotenuse')
>>> triangle = Triangle(3, 4, 5)
>>> triangle.adjacent
4
>>> print(triangle)
Triangle(opposite=3, adjacent=4, hypotenuse=5)


Even if you want to allow temporarily invalid Triangles, namedtuple is the way to go since it makes so many other things easier. For instance, since a namedtuple is just like a tuple with benefits, you could count your Nones directly:

triangle = Triangle(input_side('...'), ...)
if triangle.count(None) != 1:
    # do some error logic as appropriate

Code Snippets

triangle=Triangle()
def input_side(side_name):
    try:
        return float(raw_input('Enter length of {}:'.format(side_name)))
    except ValueError:
        return None
triangle = Triangle(
    input_side('adjacent'),
    input_side('opposite'),
    input_side('hypotenuse')
)
def __init__(self, adjacent, opposite, hypotenuse):
    self.adjacent = adjacent
    self.opposite = opposite
    self.hypotenuse = hypotenuse
def count_nones(self):
    nones=0
    for side, length in self.side.iteritems():
        if length is None:
            nones+=1
    return nones

Context

StackExchange Code Review Q#116058, answer score: 8

Revisions (0)

No revisions yet.