3
\$\begingroup\$

i am rather new at python and this is my very first project i did myself. i am wondering if there is anyway i can improve this or give me advice on making this better. thank you

import sys
valid = ["+", "-", "*", "/"]
invalid = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
def start():
    while True:
        a = input("").lower()
        for banned in a:
            if banned in invalid:
                print("error")
            else:
                sum1 = int(a)
        cal = input("")
        b = input("").lower()
        for banned in b:
            if banned in invalid:
                print("Error")
                start()
            else:
                sum2 = int(b)
        break;
    for list in cal:
        if list in valid:
            if list == "+":
                total = sum1 + sum2
                print("=" + total)
            if list == "-":
                total = sum1 - sum2
                print(total)
                start()
            if list == "*":
                total = sum1 * sum2
                print(total)
                start()
            if list == "/":
                total = sum1 / sum2
                print(total)
                start()
        else:
            print("Wrong Format")
            start()
start()
\$\endgroup\$
3
\$\begingroup\$

A few comments focused less on line-by-line details and more on how to build better command-line scripts and on simplifying logic.

How do I use this program? Your input() calls don't tell me what's going on. Help, please.

Users don't enjoy tedious interactions with computers. Would you rather type a number, press enter, type an operator, press enter, type a number, press enter ... OR just enter the darn equation? For example:

reply = input('Enter equation: INT OP INT: ')

Users really don't enjoy tedious interactions with computers. With rare exceptions, sensible command-line programs don't use input() at all -- or at least not by default. Learn about sys.argv and let your users run your calculator directly, without forcing them to become best buddies with their computer. Here's what a command-line usage might look like:

$ python simple-calculator.py 32 + 10
42

Learn about exception handling. Rather than getting bogged down in creating a data structure holding every possible bad input (very hard, especially if you consider all conceivable UTF-8 inputs), just try to do the thing you want to do -- with a safety net.

while True:
    try:
        # Each of these lines could fail. That's OK. Be optimistic!
        s1, op, s2 = reply.split()
        a = int(s1)
        b = int(s2)
        assert op in '+-*/'
        break                  # Success.
    except Exception as e:
        # The safety net.
        print('Invalid input')

Use data structures to simplify logic. Wouldn't it be cool to get rid of a pile of if-else checks and instead just do the thing you want to do directly? In such situations, a data structure can often help. What we need is a way to take an operator and then use it to get a 2-argument function that will perform the math we need. In other words, we need a mapping between operators and the corresponding math functions. Here are 3 ways to achieve that:

import operator

def add(a, b):
    return a + b

ops = {
    '+': add,                    # Define your own functions.
    '-': lambda a, b: a - b,     # Or lambdas.
    '*': operator.mul,           # Or get them from a library.
    '/': operator.truediv,
}

# With that data structure, our code would reduce to this.
answer = ops[op](a, b)     # Wrap in try-except to catch divide by 0.
print(answer)
\$\endgroup\$
1
\$\begingroup\$

Suggestion 1

Your list of invalid characters and in checks will work just as well as a string, but be shorter and more readable.

invalid = 'abcdefghijklmnopqrstuvwxyz'

Same applies to the valid array of characters.

Suggestion 2

for banned in a:

banned is a very confusing variable name for this use. You don't know if it's banned until you've compared it with the invalid list of characters. You should rather use character or letter or a shorter basic name like c or x.

Suggestion 3

a = input("").lower()

a is also a poor name here since it doesn't tell us what it is. Rather name it input1 or something similar, and of course change b to input2 or something else that is descriptive.

Suggestion 4

for list in cal:

list is not just a poorly chosen name that misleads the reader, it's also a keyword / function in Python and should not be used as a variable name in any situation. I suggest operator as a better name since you're taking in an operator for your calculation.

Suggestion 5

You're calling start() after every operation except + . This looks like a mistake. Why not for +?

Suggestion 6

total = sum1 * sum2
print(total)

For every operation, you're saving the total in a new variable and then printing it. But you don't use it for anything else than printing, so you might as well print the result directly without storing it in a variable.

print(sum1 * sum2)
\$\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.