Right triangle calculator

Question

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 "You must leave one side blank."
 quit()
 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(a*a+o*o)
 #adjacent is missing
 if a == None:
 self.side["adjacent"] = math.sqrt(h*h-o*o)
 #opposite is missing
 if o == None:
 self.side["opposite"] = math.sqrt(h*h-a*a)
#begin program
triangle=Triangle()
triangle.define_sides()
triangle.print_sides()
triangle.validate_input()
triangle.calculate_missing_length()
triangle.print_sides()

• SirPython: Welcome to Code Review! Good job on your first question.

Answer 1

I don't think this is particularly forced, and overall your code is pretty good. There are some more Pythonic things you could do, and I have a few coding conventions I like to use in Python, such as "don't initialize an object to an invalid state if you can avoid it". This applies to your Triangle object being initialized with all sides being zero, and since the purpose of this particular triangle object is to solve for one of the sides, the Triangle is created but not ready for its purpose. So I would recommend just go ahead and call the define_sides() method in the constructor, as well as the validate input (again so we don't have invalid Triangles floating around):

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

This takes care of all the business required to create a valid instance of Triangle for this purpose.

One simple Pythonic update would be to define a repr() method, which would allow you to print the triangle just like you print anything else.

def __repr__(self):
 s = ""
 for i in self.side:
 s += "side %d equals %f\n" % (i,self.side[i])
 return s

I would also update count_nones to be a little simpler

def count_nones(self):
 none_list = [0 if not value else 1 for value in a.values()]
 return sum(none_list)

One minor mod to validate_inputs() - it looks like you aren't quitting when you have too many Nones:

def validate_input(self):
 nNones=self.count_nones()
 if nNones < 1:
 print "You must leave one side blank."
 quit()
 if nNones > 1:
 print "Only one side can be left blank."
 quit() ## also quit here

So your final steps to execute your task look like this:

triangle=Triangle()
print(triangle)
triangle.calculate_missing_length()
print(triangle)

This seems like a more natural flow to me, so maybe having fewer steps to execute the task with make it seem less "forced".

By the way, I like how you use whitespace and clean indentation. Your code is very readable, which is always a top priority!

• user1717828: Thank you very much for taking the time to walk through my code with me! I will implement the heart of your suggestions in future code to try making it more Pythonic and see if that removes this feeling of "forcing" the OOP practice. Cheers.
• Caridorc: please avoid string addition in a loop. It can get veery slow
• gariepy: @Caridorc: if the OP needs to run this 1000000 times, I agree. But we're talking about a loop of 3 iterations, so I think having code that is readable for the OP is fine. There are times when it isn't worth the effort to optimize.
• Caridorc: @gariepy But the optimized code is also easier to read.

Answer 2

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:

1. Input your sides
2. Make sure exactly 2 of them are valid
3. Calculate the third
4. 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

• user1717828: Could you recommend a book or other reference that expands on this concept of once you have a Triangle then it must be valid? That feels like it gets at the heart of my discontent with the OOP as I've implemented it.
• Barry: @user1717828 Not really, sorry.
• user1717828: Alright, well thanks for the rest of the tips. I really appreciate the time you put into this.
• Gareth Rees: @user1717828: A property that is always true of an instance of a class is called a "class invariant".

Answer 3

The first question I would ask about this code (or indeed any code) is, how much of this is really necessary? All code imposes a maintenance burden in proportion to its length, so you should always be asking yourself whether it provides enough benefits to justify the burden.

If I had to represent right-angled triangles, I would start with the simplest possible representation, namely a tuple of length 3:

from math import hypot, sqrt
# If you know both short sides:
triangle = a, b, hypot(a, b)
# If you know one short side and the hypotenuse:
triangle = a, sqrt(h**2 - a**2), h

and then update the code as necessary to meet the requirements of the application, while keeping it short and simple.

• user1717828: This project is about me trying to learn OOP. You're right the project is contrived, but that's because I'm trying to learn how to build objects with member variables and functions, not actually build a triangle.
• Gareth Rees: @user1717828: I think you would get better results if you started out with a problem that was better suited to an object-oriented approach. For example, you might look at the exercises in Stephen Lott's Building Skills in Object-Oriented Design.
• user1717828: That looks like a great resource. Thanks.