6
\$\begingroup\$

For the following code, I have trouble improving it. I need to write a function getExpression(level, operator) which has one integer parameter representing a level and another string parameter representing the arithmetic operator. The function generates a string expression as well as the answer, based on the requirements for the level. Both the string expression and the result value are returned.

import random def getExpression(level, operator): # Generate operands based on the level if operator == "+": # Addition if level == 1: num1 = random.randint(1, 9) # Single digit operand num2 = random.randint(1, 9) elif level == 2: num1 = random.randint(10, 99) # Two digit operand num2 = random.randint(10, 99) elif level == 3: num1 = random.randint(100, 999) # Three digit operand num2 = random.randint(100, 999) elif level == 4: num1 = random.randint(1000, 9999) # Four digit operand num2 = random.randint(1000, 9999) elif level == 5: num1 = random.randint(10000, 99999) # Five digit operand num2 = random.randint(10000, 99999) # Return the expression and result result = num1 + num2 expression = f"{num1} + {num2} = " elif operator == "-": # Subtraction if level == 1: num1 = random.randint(2, 9) # Single digit operand num2 = random.randint(1, num1-1) # Ensure num2 is smaller than num1 elif level == 2: num1 = random.randint(10, 99) # Two digit operand num2 = random.randint(1, num1-1) # Ensure num2 is smaller than num1 elif level == 3: num1 = random.randint(100, 999) # Three digit operand num2 = random.randint(1, num1-1) # Ensure num2 is smaller than num1 elif level == 4: num1 = random.randint(1000, 9999) # Four digit operand num2 = random.randint(1, num1-1) # Ensure num2 is smaller than num1 elif level == 5: num1 = random.randint(10000, 99999) # Five digit operand num2 = random.randint(1, num1-1) # Ensure num2 is smaller than num1 # Return the expression and result result = num1 - num2 expression = f"{num1} - {num2} = " elif operator == "*": # Multiplication if level == 1: num1 = random.randint(2, 9) # Avoid starting with 1 num2 = random.randint(2, 9) elif level == 2: num1 = random.randint(10, 99) num2 = random.randint(2, 9) elif level == 3: num1 = random.randint(100, 999) num2 = random.randint(2, 9) elif level == 4: num1 = random.randint(1000, 9999) num2 = random.randint(2, 9) elif level == 5: num1 = random.randint(10000, 99999) num2 = random.randint(2, 9) # Return the expression and result result = num1 * num2 expression = f"{num1} * {num2} = " return expression, result 
\$\endgroup\$
0

    3 Answers 3

    8
    \$\begingroup\$

    getExpression should be written get_expression by PEP 8, and should have type hints. The operator is a great candidate for being typed as a Literal.

    The code is too repetitive. You need to DRY (don't repeat yourself) it up.

    This code will be simplified by use of the modern match/case syntax.

    Don't if on level (usually, other than one edge case). Just raise 10 to the power of the level.

    Don't randint; these calls are better represented by randrange.

    You need unit tests; I haven't shown any. Maybe I will later today.

    All together,

    import operator import random import typing def get_expression(level: int, symbol: typing.Literal['+', '-', '*']) -> tuple[ str, # expression without result, i.e. 1 + 2 = int, # expected value ]: if level < 1: raise ValueError(f'level must be at least 1, not {level}') low = 10**(level - 1) high = 10*low match symbol: case '+': op = operator.add a_low = low case '-': op = operator.sub if level == 1: a_low = low + 1 else: a_low = low case '*': op = operator.mul if level == 1: a_low = 2 else: a_low = low case _: raise ValueError(f'{symbol} is not a valid operator') a = random.randrange(a_low, high) match symbol: case '+': b_low = low b_high = high case '-': b_low = 1 b_high = a case '*': b_low = 2 b_high = 10 b = random.randrange(b_low, b_high) expression = f'{a} {symbol} {b} = ' expected = op(a, b) return expression, expected def demo() -> None: print(get_expression(level=2, symbol='+')) print(get_expression(level=3, symbol='-')) print(get_expression(level=6, symbol='*')) if __name__ == '__main__': demo() 
    ('59 + 56 = ', 115) ('780 - 84 = ', 696) ('860709 * 7 = ', 6024963) 
    \$\endgroup\$
    0
      6
      \$\begingroup\$

      Documentation

      The PEP 8 style guide recommends adding docstrings for functions. You can convert the function comment:

      def getExpression(level, operator): # Generate operands based on the level 

      into a docstring:

      def getExpression(level, operator): """ Generate operands based on the level """ 

      It would also be good to add details about the input types and return types.

      Naming

      PEP 8 recommends snake_case for function names.

      getExpression would be get_expression

      Validation

      Unless the code that calls your function makes sure that the inputs are valid, consider adding some simple checking to the function. For example, you should think about how to handle unexpected values passed to the level parameter. From the code, you expect it to be an integer between 1 and 5. If the function is passed an integer outside that range (like 6), the code exits with an error. The same is true if passed a floating-point value (2.1) or a string ("hello").

      DRY

      There is a lot of repeated code. For example, for the addition operator, you can take advantage of the relationship between the level and the upper and lower limits of the values passed to randint. For levels 2 and above, you can replace the hard-coded values (10, 99, 100, 999, etc.) with function calls, where you declare new functions:

      def lower_limit(level): """ Return lower limit as a power of 10 """ return 10**(level - 1) def upper_limit(level): """ Return lower limit as a power of 10, minus 1 """ return (10**level) - 1 

      This reduces the addition code to:

      if operator == "+": # Addition num1 = random.randint(lower_limit(level), upper_limit(level)) num2 = random.randint(lower_limit(level), upper_limit(level)) # Return the expression and result result = num1 + num2 expression = f"{num1} + {num2} = " 

      This eliminates much of the repetition, and it allows the code to scale to more values of level. These functions can also be used with the other operators.

      \$\endgroup\$
      2
      • 1
        \$\begingroup\$Why do you add leading and trailing blanks to the docstring: """ Generate operands based on the level """?\$\endgroup\$CommentedMar 17 at 14:53
      • 1
        \$\begingroup\$@TimurShtatland: I think it is easier to read with the single space, don't you?\$\endgroup\$
        – toolic
        CommentedMar 17 at 14:54
      2
      \$\begingroup\$

      I'd keep it simple, mainly remove your level-based repetition. Compute basic low and high number for the level, then tweak where needed. The expression can use the given operator, no need to write it once for each case. The result could be computed with a single eval at the end as well, but each result = ... is painless and I like preparing each case's values (the operands and the result) in one place. For randint, I find random. redundant, so I'd avoid that.

      from random import randint def getExpression(level, operator): low = 10 ** (level-1) high = 10**level - 1 if operator == "+": num1 = randint(low, high) num2 = randint(low, high) result = num1 + num2 elif operator == "-": num1 = randint(max(low, 2), high) num2 = randint(1, num1-1) result = num1 - num2 elif operator == "*": num1 = randint(max(low, 2), high) num2 = randint(2, 9) result = num1 * num2 expression = f"{num1} {operator} {num2} = " return expression, result 
      \$\endgroup\$

        Start asking to get answers

        Find the answer to your question by asking.

        Ask question

        Explore related questions

        See similar questions with these tags.