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

Python Trig Calculator

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

Problem

It's big, it's ugly, but it should work. My question is: how could I have implemented the "typeselection" function so it is less repetitive? Any tips on other improvements in coding style are welcomed as well.

```
import math
import turtle
class Triang:

#MATHS FUNCTIONS; THESE GET CALLED WHEN PROGRAM HAS DECIDED ON TRIANGLE TYPE

def dass(self, arg, arg1, arg2, ans, args, args1, args2):
arg2=math.radians(arg2)
arg1=math.radians(arg1)
print (ans, "= ( ", args, "* sin (", args1, ") / ( sin (", args2, ") )")
a=(arg*math.sin(arg1))/math.sin(arg2)
print (ans, "=", a)
return a

def sss1(self, arg, arg1, arg2, ans, args, args1, args2):
print (ans, "= acos ( (", args, "^ 2 +", args1, "^ 2 -", args2, "^ 2 ) / ( 2 ", args, "", args1, ") )")
a=math.degrees(math.acos((arg2+arg12-arg2**2)/(2argarg1)))
print (ans, "=", a)
return a

def sas(self, arg, arg1, arg2, ans, args, args1, args2):
arg2=math.radians(arg2)
print (ans, "^ 2 =", args, "^ 2 +", args1, "^ 2 - 2 ", args1, " cos ( ", args2, ")")
a=(arg2+arg12-2arg1arg1*math.cos(arg2))
print (ans, "^ 2 =", a)
if a = 3 :
self.sides()
else :
self.angles()

def obtusecol(self):
self.oa = self.a if self.a else False
self.ob = self.b if self.b else False
self.oc = self.c if self.c else False
self.oab = self.ab if self.ab else False
self.oac = self.ac if self.ac else False
self.obc = self.bc if self.bc else False

def collector(self, arg, arg1, arg2="A"):
print (" ")
print ("Enter the value for,", arg1, arg)
if arg1=="side":
print ("Some people would call that side", arg2)

a=input(">>")
try:
a=float(a)
except ValueError:
if a=="x" or a=="X":
a=False

else:

Solution

Just general style

What the other comment said is true, the style can be improved here.

  • meaningful names (Calling arguments arg is not so helpful :) For


the mathematical stuff, there are often already established names
for the parameters to functions which you can use.)

  • logging is ok with print, but I would not introduce additional


parameters for it. (The code calling this can't observe the difference
between whether the "ab" or "ac" string is passed -- this makes little
sense for that caller.)

  • the user interaction should be separated from the math, ideally outside


the class, so that you can focus on either math or UI, and have a smaller
piece of code to look at for each.

Taking apart these cases

In the typeselection method, you can still simplify many of your cases by thinking about the math.

For example, take this code. In this case, we already know that the angles a, b, c are given.

if self.a and self.b and self.c:
    if self.ab and not self.bc and not self.ac:  # Case 1
        self.deter()                
        self.ac=self.dass(self.ab, self.b, self.c, "ac", "ab", "b", "c")
        self.bc=self.dass(self.ac, self.a, self.b, "bc", "ac", "a", "b")
    elif self.bc and not self.ab and not self.ac:  # Case 2
        self.deter() 
        self.ab=self.dass(self.bc, self.c, self.a, "ab", "bc", "c", "a")
        self.ac=self.dass(self.bc, self.b, self.a, "ac", "bc", "b", "a")
    elif self.ac and not self.bc and not self.ab:  # Case 3
        self.deter() 
        self.ab=self.dass(self.ac, self.c, self.b, "ab", "ac", "c", "b")
        self.bc=self.dass(self.ac, self.a, self.b, "bc", "ac", "a", "b")
    elif self.ab and self.bc and not self.ac:  # Case 4
        self.deter() 
        self.ac=self.dass(self.bc, self.b, self.a, "ac", "bc", "b", "a")
    elif self.ab and self.ac and not self.bc:  # Case 5
        self.deter() 
        self.bc=self.dass(self.ab, self.a, self.c, "bc", "ab", "a", "c")
    elif self.bc and self.ac and not self.ab:  # Case 6
        self.deter()
        self.ab=self.dass(self.ac, self.c, self.b, "ab", "ac", "c", "b")


In cases 4, 5 and 6, you already have all angles and two lengths of the triangle given. These may conflict already with each other. You may collapse this with one of the other cases above if you want to ignore that, or you check for the conflict. So we are getting a bit tangled up in all the different cases here... :)

Another approach: Loop until you can't apply any of the rules any more

Look at what you need for each individual calculation:

self.ac=self.dass(self.ab, self.b, self.c, "ac", "ab", "b", "c")


This just needs ab, b and c to work, and assumes that ac is not known.
But the code is also guarded by other assumptions right now. You could try to only guard it by the things you really need (ab, b and c are there, ac is not), and try out all the formulas until you can't do any changes any more. :)

while True:
  if not self.ac:
    if self.ab and self.b and self.c:
      self.ac = self.dass(self.ab, self.b, self.c)
      continue  # Next loop iteration
    elif self.bc and self.b and self.a:
      self.ac = self.dass(self.bc, self.b, self.a)
      continue  # Next loop iteration
    # etc.
  # etc.; make sure to get all cases
  break  # Break out of loop


Then each way to calculate ac would only need to be mentioned once. You can also take the whole part within if not self.ac and extract it into a single method where the names of the corner points are exchanged. (Replace self.xxx with the parameters of that function.)

Constraint propagation, if you want to get fancy

There is also a way to calculate this "lazily" without a giant while True loop, but that's much more difficult. I wouldn't recommend it to you now, but I'll still mention it because it's fancy, and you may want to revisit in the future. It takes the "keep track of constraints between things" problem to another level of abstraction. :)
http://mitpress.mit.edu/sicp/full-text/book/book-Z-H-22.html#%_sec_3.3.5

This is basically a graph of present or non-present values which are connected to each other through mathematical transformations. For instance, you might have the graph nodes X, Y and Z with the connecting transformation that says "X * Y = Z". Now the transformation is represented as an object and gets notified whenever one of the values is suddenly available. Once there are two values available, the transformation will calculate the third value and set it. For instance, when we know that Y=3 and Z=15, it will calculate and set X to 5 (but it will do division or multiplication, depending on which inputs it gets! :)). Then when it sets X to 5, there may be other transformations in the network that now get triggered because they were waiting for a value for X. Another nice upside to this is that the network can detect when there are conflicting inputs in the system.

Code Snippets

if self.a and self.b and self.c:
    if self.ab and not self.bc and not self.ac:  # Case 1
        self.deter()                
        self.ac=self.dass(self.ab, self.b, self.c, "ac", "ab", "b", "c")
        self.bc=self.dass(self.ac, self.a, self.b, "bc", "ac", "a", "b")
    elif self.bc and not self.ab and not self.ac:  # Case 2
        self.deter() 
        self.ab=self.dass(self.bc, self.c, self.a, "ab", "bc", "c", "a")
        self.ac=self.dass(self.bc, self.b, self.a, "ac", "bc", "b", "a")
    elif self.ac and not self.bc and not self.ab:  # Case 3
        self.deter() 
        self.ab=self.dass(self.ac, self.c, self.b, "ab", "ac", "c", "b")
        self.bc=self.dass(self.ac, self.a, self.b, "bc", "ac", "a", "b")
    elif self.ab and self.bc and not self.ac:  # Case 4
        self.deter() 
        self.ac=self.dass(self.bc, self.b, self.a, "ac", "bc", "b", "a")
    elif self.ab and self.ac and not self.bc:  # Case 5
        self.deter() 
        self.bc=self.dass(self.ab, self.a, self.c, "bc", "ab", "a", "c")
    elif self.bc and self.ac and not self.ab:  # Case 6
        self.deter()
        self.ab=self.dass(self.ac, self.c, self.b, "ab", "ac", "c", "b")
self.ac=self.dass(self.ab, self.b, self.c, "ac", "ab", "b", "c")
while True:
  if not self.ac:
    if self.ab and self.b and self.c:
      self.ac = self.dass(self.ab, self.b, self.c)
      continue  # Next loop iteration
    elif self.bc and self.b and self.a:
      self.ac = self.dass(self.bc, self.b, self.a)
      continue  # Next loop iteration
    # etc.
  # etc.; make sure to get all cases
  break  # Break out of loop

Context

StackExchange Code Review Q#89958, answer score: 7

Revisions (0)

No revisions yet.