Below is the code I wrote to perform basic Trigonometry without using the math module(except for 1 place) for the purpose of teaching myself basic trig and to improve my python. I have never taken a trig class, so please consider this fact when explaining some fallacies in my trig logic, and please help me correct it.
I would like feedback on
- The architecture of the code. I find my use of dictionaries handling the return of data very restrictive and would like to know how better to handle output or returning of values.
- The usage of **kargs, is this proper utilization? Is there a different way to write these functions where I can input large amounts of (undermined) data?
- Lines 47-64, better ways to write the block of if/elif statements for readability and functionality.
#!/usr/bin/env python3
import math
def sin(*args, **kwargs):
degrees = kwargs.get('degrees', None)
opposite = kwargs.get('opposite', None)
hypotenuse = kwargs.get('hypotenuse', None)
if degrees:
return math.sin(math.radians(degrees))
elif opposite and hypotenuse:
return(opposite / hypotenuse)
def cos(*args, **kwargs):
degrees = kwargs.get('degrees', None)
adjacent = kwargs.get('adjacent', None)
hypotenuse = kwargs.get('hypotenuse', None)
if degrees:
return math.cos(math.radians(degrees))
elif adjacent and hypotenuse:
return(adjacent / hypotenuse)
def tan(*args, **kwargs):
degrees = kwargs.get('degrees', None)
adjacent = kwargs.get('adjacent', None)
opposite = kwargs.get('opposite', None)
if degrees:
return math.tan(math.radians(degrees))
elif opposite and adjacent:
return(opposite / adjacent)
def find_missing_side(*args, **kwargs):
sin = kwargs.get('sin', None)
cos = kwargs.get('cos', None)
tan = kwargs.get('tan', None)
opposite = kwargs.get('opposite', None)
hypotenuse = kwargs.get('hypotenuse', None)
adjacent = kwargs.get('adjacent', None)
if sin and hypotenuse:
opposite = sin * hypotenuse
return {'opposite': opposite}
elif sin and opposite:
hypotenuse = opposite / sin
return {'hypotenuse': hypotenuse}
elif cos and adjacent:
hypotenuse = adjacent / cos
return {'hypotenuse': hypotenuse}
elif cos and hypotenuse:
adjacent = cos * hypotenuse
return {'adjacent': adjacent}
elif tan and opposite:
adjacent = opposite / tan
return {'adjacent': adjacent}
elif tan and adjacent:
opposite = tan * adjacent
return {'opposite': opposite}
def pythagorean(*args, **kwargs):
opposite = kwargs.get('opposite', None)
hypotenuse = kwargs.get('hypotenuse', None)
adjacent = kwargs.get('adjacent', None)
if hypotenuse and opposite:
adjacent = math.sqrt(hypotenuse * hypotenuse - opposite * opposite)
elif adjacent and opposite:
hypotenuse = math.sqrt(adjacent * adjacent + opposite * opposite)
elif hypotenuse and adjacent:
opposite = math.sqrt(hypotenuse * hypotenuse - adjacent * adjacent)
return {'hypotenuse': hypotenuse, 'adjacent': adjacent,
'opposite': opposite}
if __name__ == "__main__":
# Testing sin, cos, tan functions.
print(sin(degrees=35)) # Answer is .57
print(sin(opposite=2.8, hypotenuse=4.9))
print(cos(degrees=35)) # Answer is .82
print(cos(adjacent=4.0, hypotenuse=4.9))
print(tan(degrees=35)) # Answer is .70
print(tan(opposite=2.8, adjacent=4.0))
# Testing find_missing_side function.
print(find_missing_side(sin=sin(degrees=35), opposite=2.8))
print(find_missing_side(cos=cos(degrees=35), hypotenuse=4.9))
print(find_missing_side(tan=tan(degrees=35), adjacent=4.0))
# Testing pythagorean function.
hypotenuse = find_missing_side(sin=sin(degrees=35), opposite=2.8)
print(pythagorean(opposite=2.8, hypotenuse=hypotenuse['hypotenuse']))
1 Answer 1
All of your arguments accept
*args
but don't use it. If someone calls your functions likesin(some_angle)
, it's going to returnNone
which is confusing. You can simply leave it out,**kwargs
still works.This is more a situation that calls for keyword-only arguments, because each function only has a small fixed number of possible arguments. This leads to better errors if, say, you misspell one of the argument names in a call. If you want to use keyword-only arguments without
*args
, you can add a single*,
before the arguments, like this:def sin(*, degrees=None, opposite=None, hypotenuse=None): if degrees: return math.sin(math.radians(degrees)) elif opposite and hypotenuse: return opposite / hypotenuse
Testing for possible arguments should be done with
if x is not None
instead ofif x
: gettingsin(degrees=0)
is totally valid but will returnNone
if you test fordegrees
instead ofdegrees is not None
!You're sort of overloading each function based on arguments passed. This makes the functions error-prone to use and possibly confusing. You can call
sin(degrees=60, opposite=1, hypotenuse=1)
and your program won't complain. You can callsin()
and getNone
.A possible solution would be to split the first four functions out like this:
def sin_from_degrees(degrees) def sin_from_edges(*, opposite, hypotenuse) def cos_from_degrees(degrees) def cos_from_edges(*, adjacent, hypotenuse) def tan_from_degrees(degrees) def tan_from_edges(*, opposite, adjacent) def opposite_from_sin(*, sin, hypotenuse) def opposite_from_tan(*, tan, adjacent) def adjacent_from_cos(*, cos, hypotenuse) def adjacent_from_tan(*, tan, opposite) def hypotenuse_from_sin(*, sin, opposite) def hypotenuse_from_cos(*, cos, adjacent)
Note how none of the arguments are optional any more, and if you try to use a function the wrong way, Python will give you a nice error message explaining exactly what went wrong. I also made
degrees
a regular argument, because I felt thatsin_from_degrees(degrees=degrees)
would be a bit redundant and there's no confusion about which argument means what if there's only one, although I can understand wanting the API to be more consistent.Your last two function is interesting in that it is overloaded to an extent, but not as easy to extricate as the other functions. I would use a namedtuple instead of a dictionary, and definitely signal if
pythagorean
is called with the wrong arguments.from typing import NamedTuple class Triangle(NamedTuple): adjacent: float opposite: float hypotenuse: float def pythagorean(*, adjacent=None, opposite=None, hypotenuse=None): if hypotenuse is not None and opposite is not None and adjacent is None: adjacent = math.sqrt(hypotenuse * hypotenuse - opposite * opposite) elif adjacent is not None and opposite is not None and hypotenuse is None: hypotenuse = math.sqrt(adjacent * adjacent + opposite * opposite) elif hypotenuse is not None and adjacent is not None and opposite is None: opposite = math.sqrt(hypotenuse * hypotenuse - adjacent * adjacent) else: raise TypeError("pythogorean() expects exactly two arguments from 'adjacent', 'opposite', and 'hypotenuse'") return Triangle(adjacent, opposite, hypotenuse)
There is still a bit of duplication (
if a is not None and b is not None and c is None: c = math.sqrt(a * a ± b * b)
occurs three times) but I'm not sure at the moment how to cleanly refactor that, especially due to the sign change when calculating the hypotenuse. You could make it a bit shorter by using comparator chaining (a is not None is not b
) but honestly I think that would be hurt readability and maintainability more than it would help.
Maybe I've gone a bit overboard with suggesting changing things you didn't ask about, but I hope that at least answers your questions!
math.sin
,math.cos
,math.tan
, 3math.radians
and 3math.sqrt
. Isn't 1+1+1+3+3 = 9 not 1? \$\endgroup\$