6
\$\begingroup\$

All the calculator programs I have seen so far seem long, complicated and they can only operate on two numbers. Mine is simple, short, and it can operate on as many numbers as you want. Please tell me if there is another way to improve this section of code.

x=[]
amount=int(input("How many numbers?"))
operation=input("(*), (/), (+), (-)")
previous1 = 0
previous2=1
for i in range(amount):
    number=int(input("Number: "))
    x.append(number)
if operation == "+":
    for i in range(amount):
        previous1=x[i]+previous1
elif operation == "*":
    for i in range(amount):
        previous2=x[i]*previous2
elif operation == "-":
    for i in range(amount):
        previous1=x[i]-previous1
elif operation == "/":
    for i in range(amount):
        previous2=x[i]/previous2
if operation == "+" or operation == "-":
    print(previous1)
else:
    print(previous2)
\$\endgroup\$
  • 3
    \$\begingroup\$ Hmm, you cannot support mixed operations. That's a heavy restriction for a generic calculator. A good improvement would be building an AST, and walk it to evaluate the parsed expressions. \$\endgroup\$ – πάντα ῥεῖ Dec 8 '15 at 20:55
  • \$\begingroup\$ Thought about that but also thought that you could do it in steps, seeing as you could see the previous answer to the previous part of your sum \$\endgroup\$ – Joe Dec 8 '15 at 21:05
  • \$\begingroup\$ @Joe, Extending it to have mixed operations is a good idea, but not trivial to do as you get into cases like 3 + 4 * 5, where most people would say that the answer is 23. If you parse continuously you get 7*5 equaling 35. So it is good exercise to extend it, but you need to implement some sort of parser to acknowledge the different associativity of operators. \$\endgroup\$ – holroy Dec 8 '15 at 21:35
  • \$\begingroup\$ @holroy Unless you just cheat and use eval() :) \$\endgroup\$ – Barry Dec 8 '15 at 22:34
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Dec 9 '15 at 22:02
5
\$\begingroup\$

reduce

Whenever you need to fold an operation across a list, like you're doing, you want to use reduce(). That takes a function, an iterable, and an optional initializer, and performs the operation across all of them. So:

reduce(f, [x, y, z])

is equivalent to:

f(f(x, y), z)

This is precisely what we want. The operator library additionally gives us all the mathematical operators, so we don't have to write them ourselves. So the various operations are:

+: reduce(operator.add, numbers) # or just sum(numbers)
-: reduce(operator.sub, numbers, 0)
*: reduce(operator.mul, numbers)
/: reduce(operator.floordiv, numbers, 1)

Simplifying the logic

We could directly translate the above into a dictionary of operators and a dictionary of initializers:

operators = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.floordiv
}

initializers = {
    '-': 0,
    '/': 1
}

result = reduce(operators[operation],
    numbers, 
    initializers.get(operation, None))

This will additionally fix your bug where for subtraction you're really adding.

List Comprehensions

It's more direct to simply initialize your numbers directly:

numbers = [int(input("Number: ")) for _ in range(amount)]

Note that x is not a good name. Prefer more descriptive names.

Division

All of your numbers are ints. Which means that when you divide, you're going to end up with 0 as soon as you have any number that isn't 1. That isn't particularly interesting, so I'd suggest using floating point numbers instead.

\$\endgroup\$
3
\$\begingroup\$

Naming

I don't like to start out a review with picking on naming, but these variable names are awful:

x=[]
amount=int(input("How many numbers?"))
previous1 = 0
previous2=1

Some renaming ideas:

  • x -> numbers
  • amount -> count
  • previous1, previous2 -> result (more on this later)

Loop over elements rather than indexes

The index variable i is unnecessary and not useful:

for i in range(amount):
    previous1=x[i]+previous1

Better to iterate directly over the elements:

for num in x:
    previous1=x[i]+previous1

Augmented assignment

Note that this:

previous1 = x[i] + previous1

Is equivalent to this (thanks to commutativity with respect to addition):

previous1 = previous1 + x[i]

This latter form can be simplified as:

previous1 += x[i]

Use sum

If you want to add many numbers, you don't need a loop:

previous1 = sum(x)

Strange -

Given numbers \$x_1, x_2, ..., x_n\$, your implementation performs something like \$x_n - (x_{n-1} - (... - (x_2 - x_1))))...)\$ which seems pretty odd. For example if the input is [1, 2, 3, 4] it performs 4 - (3 - (2 - 1)) which I'm not sure how is useful...

Poor formatting

Please follow PEP8, the Python style guide.

Suggested implementation

It would be good to move the calculation logic to a function, and separate it from the input reading. While at it, some doctests would be interesting too.

With the above suggestions applied (and then some more):

def calculate(numbers, operation):
    """
    >>> calculate([1, 2, 3, 4], '+')
    10
    >>> calculate([1, 2, 3, 4], '-')
    2
    >>> calculate([1, 2, 3, 4], '*')
    24
    >>> calculate([1, 2, 3, 4], '/')
    2.6666666666666665
    """

    if operation == "+":
        return sum(numbers)

    if operation == "*":
        result = 1
        for num in numbers:
            result *= num
        return result

    if operation == "-":
        result = 0
        for num in numbers:
            result = num - result
        return result

    if operation == "/":
        result = 1
        for num in numbers:
            result = num / result
        return result

    raise Exception('Unsupported operator')


def main():
    count = int(input("How many numbers?"))
    operation = input("(*), (/), (+), (-)")
    numbers = [int(input("Number: ")) for _ in range(count)]
    print(calculate(numbers, operation))
\$\endgroup\$

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.