5
\$\begingroup\$

I have some code for a calculator, and I was wondering what I could look at to make the code smaller, easier to read or better in any way. I'm particularly looking at my global variable x. Is there any way to change the code to completely remove that? Someone mentioned a getTwoInputs() function, but with multiple different questions (powers() input and addition() input) I assume it would take up the same space.

def Addition():
    print('Addition: What are your numbers?')
    a = float(input('First Number:'))
    b = float(input('Second Number:'))
    print('Your Answer is:', a + b)


def Subtraction():
    print('Subtraction: What are your numbers?')
    c = float(input('First Number:'))
    d = float(input('Second Number:'))
    print('Your Answer is:', c - d)


def Multiplication():
    print('Multiplication: What are your numbers?')
    e = float(input('First Number:'))
    f = float(input('Second Number:'))
    print('Your Answer is:', e * f)


def Division():
    print('Division: What are your numbers?')
    g = float(input('First Number:'))
    h = float(input('Second Number:'))
    print('Your Answer is:', g / h)


def Roots():
    print('Roots: What do you want to Root?')
    i = float(input('Number:'))
    print('Roots: By what root?')
    j = float(input('Number:'))
    k = ( 1 / j )
    print('Your Answer is:',i**k)


def Powers():
    print('Powers: What number would you like to multiply by itself?')
    l = float(input('Number:'))
    print('Powers: How many times would you like to multiply by itself?')
    m = float(input('Number:'))
    print('Your Answer is:',l**m)

x = 'test'


def Question():
        print('What would you like to do?')
        x = input('(Add, Subtract, Divide, Multiply, Powers, Roots or Quit)')
        if x.lower().startswith('a'):
            x = 'test123'
            print(Addition())
            x = 'test'
        elif x.lower().startswith('d'):
            x = 'test123'
            print(Division())
            x = 'test'
        elif x.lower().startswith('m'):
            x = 'test123'
            print(Multiplication())
            x = 'test'
        elif x.lower().startswith('s'):
            x = 'test123'
            print(Subtraction())
            x = 'test'
        elif x.lower().startswith('r'):
            x = 'test123'
            print(Roots())
            x = 'test'
        elif x.lower().startswith('p'):
            x = 'test123'
            print(Powers())
            x = 'test'
        elif x.lower().startswith('e'):
            x = 'test'
            print(exit())
        elif x.lower().startswith('q'):
            x = 'test'
            print(exit())
        else:
            print("""
Please type the first letter of what you want to do.
""")
            print(Question())



while x == 'test':
    Question()
\$\endgroup\$
5
\$\begingroup\$

I am not sure it is very clear to you what your different functions are returning as you seem to be trying to print their return value even though they do not return anything special.

You do not need to change your variable names in each function without special reason. a and b are decent names in your case, you don't need to use c and d later and then e and f as there is no way to get confused anyway.

The way you use your x variable is pretty cryptic to me. I guess this is just some testing/debugging but it would help if you could submit a clean version of your code for reviewing.

Most of what happens in your function is (pretty much) the same from one to another. A good thing to do is to try to avoid repetition whenever you can and to abstract common behaviors. The quick and dirty way I've used to avoid such a repetition was to notice that you ask the user for 2 numbers anyway so it doesn't really matter which operator we will use them for. My minimalist solution stores things in a dictionary (you could improve the content of the dictionary if you wanted to have more things changing from one operator to another such as user prompts).

#!/usr/bin/python
import operator

operations={
        'a':('Addition',      operator.add),
        's':('Substraction',  operator.sub),
        'm':('Multiplication',operator.mul),
        'd':('Division',      operator.truediv),
        'p':('Power',         operator.pow),
        'r':('Root',          lambda x, y:x**(1/y)),
}

while True:
        # TODO : Next line could be generated from the list if it keeps growing
        x = input('(Add, Subtract, Divide, Multiply, Powers, Roots or Quit)').lower()
        if x in operations:
                operation_name,operator=operations[x]
                print(operation_name)
                a = float(input('First Number:'))
                b = float(input('Second Number:'))
                print("Your answer is ",operator(a,b))
        elif x.startswith('q'):
                break
        else:
                print("Please type the first letter of what you want to do.")
\$\endgroup\$
  • \$\begingroup\$ Comment taken into account :-) \$\endgroup\$ – Josay May 2 '13 at 8:18
  • \$\begingroup\$ One of the most important improvements here is that the real work (the operation dictionary) is separated from the interface/controller (the while loop). Just pointing it out for @Rinslep. \$\endgroup\$ – Quentin Pradet May 2 '13 at 8:48
1
\$\begingroup\$

Personally, I find long blocks of if statements to be difficult to read and maintain. Instead, I populated a dictionary with function handles, pointing to each of your arithmetic operations. I then added a second set of keys, but taking each of your function names, grabbing the first letter of the name, and making it lower case. That way, you can choose the add operation with either "Add" or "a", just like you had designed it before.

It wasn't clear if the code snippet you posted was supposed to run as part of a larger application, or if it was intended to run as it's own, self-contained "main" script, but I added if __name__=="__main__" to the bottom of the script so that it could be run by itself.

def Addition():
    print('Addition: What are your numbers?')
    a = float(input('First Number:'))
    b = float(input('Second Number:'))
    print('Your Answer is:', a + b)


def Subtraction():
    print('Subtraction: What are your numbers?')
    c = float(input('First Number:'))
    d = float(input('Second Number:'))
    print('Your Answer is:', c - d)


def Multiplication():
    print('Multiplication: What are your numbers?')
    e = float(input('First Number:'))
    f = float(input('Second Number:'))
    print('Your Answer is:', e * f)


def Division():
    print('Division: What are your numbers?')
    g = float(input('First Number:'))
    h = float(input('Second Number:'))
    print('Your Answer is:', g / h)


def Roots():
    print('Roots: What do you want to Root?')
    i = float(input('Number:'))
    print('Roots: By what root?')
    j = float(input('Number:'))
    k = ( 1 / j )
    print('Your Answer is:',i**k)


def Powers():
    print('Powers: What number would you like to multiply by itself?')
    l = float(input('Number:'))
    print('Powers: How many times would you like to multiply by itself?')
    m = float(input('Number:'))
    print('Your Answer is:',l**m)


def Question():
    print('What would you like to do?')
    operations = {"Addition": Addition, 
                  "Subtraction": Subtraction, 
                  "Multiply": Multiplication,
                  "Powers": Powers,
                  "Roots": Roots,
                  "Quit": exit}
    operations.update(dict([(key[0].lower(), operations[key]) for key in operations]))
    while True:
        try:
            x = raw_input('(Add, Subtract, Divide, Multiply, Powers, Roots or Quit)')
            op = operations[x]()
        except KeyError:
            print("Please type the first letter of what you want to do.")


if __name__=="__main__":
    Question()
\$\endgroup\$
  • \$\begingroup\$ when i run this, wont i need something to stop the endless printing of "Please type the first letter of what you want to do."? \$\endgroup\$ – Rinslep May 1 '13 at 21:36
  • \$\begingroup\$ No, it will only be displayed once. If they choose an invalid option, it will print the error message, and then prompt them to choose a valid option. \$\endgroup\$ – John Thomas Evans May 1 '13 at 21:44
  • \$\begingroup\$ just tried it, prompted me about 2000 times in a matter of seconds... not sure why... \$\endgroup\$ – Rinslep May 1 '13 at 22:44
  • \$\begingroup\$ That's strange. raw_input should be blocking method. I've tried my code on two different computers, one running Python 2.5, and another one running 2.7, and both ran the code without any problems. I noticed that you stated in the question that you were using Python 3.3. Perhaps they've changed raw_input() so that it's non-blocking. \$\endgroup\$ – John Thomas Evans May 1 '13 at 22:57
  • \$\begingroup\$ raw_input has been renamed input in Python 3. This program stops at a NameError on Python 3. \$\endgroup\$ – Janne Karila May 2 '13 at 6:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.