patternpythonMinor
Right triangle calculator
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
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:
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
Building up our Triangle
To that end, let's consider
Catching
Once we have that, we can use that to call into our
where we rewrite the constructor to be:
It is much better to simply have your members as members than to hide than in a dictionary.
Input Validation
Comparison against
though the above can be abridged into:
Or, now that I'm proposing we not use a dictionary:
But really, you should do all of this external to your
Printing
In Python, you would typically add a
If you follow where I'm going up to now - you'll see I've made
Even if you want to allow temporarily invalid
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 NoneCatching
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 = hypotenuseIt 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 nonesthough 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
Trianglewith 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
)namedtupleIf 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 appropriateCode Snippets
triangle=Triangle()def input_side(side_name):
try:
return float(raw_input('Enter length of {}:'.format(side_name)))
except ValueError:
return Nonetriangle = Triangle(
input_side('adjacent'),
input_side('opposite'),
input_side('hypotenuse')
)def __init__(self, adjacent, opposite, hypotenuse):
self.adjacent = adjacent
self.opposite = opposite
self.hypotenuse = hypotenusedef count_nones(self):
nones=0
for side, length in self.side.iteritems():
if length is None:
nones+=1
return nonesContext
StackExchange Code Review Q#116058, answer score: 8
Revisions (0)
No revisions yet.