13
\$\begingroup\$

In a university library, there was copy of "Making The Most Of Your ZX 81" by Tim Hartnell. I flipped to an interesting page from the index and found this program in BASIC, and thought it interesting enough to rewrite in Python. It's a number guessing game.

from random import random
from sys import exit
print("LOST IN SPACE:\nYou have fifteen hours to find a capsule lost in a SEVEN kilometer cube of space.\n")
def main():
    def winCondition(i):
            print("You found it with {} hours of air left".format(15 - i))
            end()
    def end():
        print("Play again? Y/N")
        response = input()
        if response.lower() == "y":
            game()
        else:
            exit(0)
    def game():
        A = int(7 * random() + 1)
        B = int(7 * random() + 1)
        C = int(7 * random() + 1)
        """
        Ripped from https://stackoverflow.com/questions/57813858/iteratively-assigning-variables-in-python/57813901#57813901
        """
        def get_input(axis):
            while True:
                user_input = input(f"{axis}-Axis: ")
                try:
                    axis_int = int(user_input)
                    assert(axis_int <= 7)
                except:
                    print("Enter a natural number in the range!")
                    continue
                else:
                    break
            return axis_int
        for i in range(15):
            print("Input 3 search coordinates")
            D, E, F = [get_input(axis) for axis in "YXZ"]
            print(str(D) + str(E) + str(F))
            if A == D and B == E and F == C:
                winCondition(i)
            else:
                print("You have {} hours of air left.".format(15 - i))
                if A > D:
                    print("UP")
                elif A < D:
                    print("DOWN")
                if B > E:
                    print("PORT")
                elif B < E:
                    print("STARBOARD")
                if C > F:
                    print("FORWARD")
                elif C < F:
                    print("BACKWARD")
                if i == 14:
                    print("Choose your next move carefully... HYPOXIA IMMINENT.")
        print("Fail, astronaut dead; capsule was at coords {}.{}.{}".format(A, B, C))
        end()
main()

I first of all want to improve concision before anything else.

\$\endgroup\$
  • 4
    \$\begingroup\$ Can you provide the original BASIC source for comparison? How important is it to you to be compatible with the original? \$\endgroup\$ – Austin Hastings Sep 6 at 0:19
  • \$\begingroup\$ It isn't important that it compares to the original. It just does the same thing. \$\endgroup\$ – JohnnyApplesauce Sep 6 at 1:36
  • 1
    \$\begingroup\$ Hmm. First thing’s first: The program doesn’t do anything. It prints a message and then immediately exits. \$\endgroup\$ – Konrad Rudolph Sep 6 at 12:16
  • 1
    \$\begingroup\$ As opposed to the reboot of the TV show Lost in Space where all the characters are played by pythons. \$\endgroup\$ – xdhmoore Sep 7 at 1:36
15
\$\begingroup\$

Nested functions

Not really sure what your motivation was, here, but this Russian nesting doll of functions is not really necessary. Under certain narrow circumstances, like limiting the number of publicly visible methods in a library, it might be called-for - but not really here. You don't need closures for this code, so just move things up to global namespace.

Variable case

A-C should be lower-case because they aren't types.

Random unpacking

a, b, c = (int(7*random() + 1) for _ in range(3))

Never except:

Currently, you can't Ctrl+C out of the program, because you're catching all exceptions. Narrow your exception type.

Don't materialize generators

D, E, F = [get_input(axis) for axis in "YXZ"]

should be

d, e, f = (get_input(axis) for axis in "YXZ")

because you don't need to keep that list in memory.

Format strings

print(str(D) + str(E) + str(F))

becomes

print(f'{d}{e}{f}')

Stack abuse

You call game, which calls end, which calls game... Don't do this. Just loop.

\$\endgroup\$
  • \$\begingroup\$ The last suggestion assumes OP is using Python >= 3.6 (they should, but they don't necessarily have to). \$\endgroup\$ – DeepSpace Sep 7 at 9:21
  • \$\begingroup\$ Because end() is the last function call in game doesn't that allow the interpreter to make a tail-call optimization? \$\endgroup\$ – Emanuel Vintilă Sep 7 at 10:54
  • \$\begingroup\$ @EmanuelVintilă Python (or at least CPython, as far as my knowledge) does not optimize rail recursive calls. \$\endgroup\$ – Blue Sep 7 at 11:50
  • \$\begingroup\$ I accepted this one because you reminded me of the call stack. I shudder. \$\endgroup\$ – JohnnyApplesauce Sep 11 at 23:25
9
\$\begingroup\$

In python, code clarity and readability is more important than conciseness. If you want concise code, go play code-golf. However, sometimes they align a bit.

This is what I'd make of it, and why I would make the changes:

from random import random
import sys  # sys.exit is a different function than the builtin exit. Importing it by name will rename that builtin
# for this script, which should be avoided. Instead, we'll use sys.exit() explicitly.


def get_input(axis):  # De-nest this. Flat is better than nested.
    while True:
        position = input(f"{axis}-Axis: ")  # Short, clear variable naming
        try:
            position = int(position)  # this will raise a ValueError when it's not a valid int
            if not 0 < position < 8:  # same as 1 <= position <= 7. Use whichever one you like.
                raise ValueError  # ValueError is for invalid values, like out of range.
        except ValueError:  # Only catch strings which aren't ints and ints out of range. That's the only expected problem here, so other Exceptions should propagate.
            print("Please enter an integer between 1 and 7 inclusive.")
            continue
        return position  # Why use else: break? Returning also breaks.


def game():  # Module level function. Avoid deep nesting if you can.
    """
    Ripped from https://stackoverflow.com/questions/57813858/iteratively-assigning-variables-in-python/57813901#57813901
    """  # Looks like a docstring. Let's position it correctly
    a, b, c = (int(7 * random() + 1) for _ in range(3))  # Like @Reinderien said
    # I'd prefer meaningful names here as well, but it's short enough that it's not a huge issue, and hard to come up
    # with.
    print("LOST IN SPACE:\nYou have fifteen hours to find a capsule lost in a SEVEN kilometer cube of space.\n")
    # Intro for every new game, instead of only the first game.
    for turn in range(5):  # Variable names should be meaningful
        # 15 turns is enough to figure out a 2^15 = 32768 Kilometer cubed space, since you can solve all axis at the 
        # same time. 5 turns is plenty. If you go for 15 turns, you can return only 1 distance identifier to make it
        # more challenging. Simply continue after printing any of them.
        print(f"You have {5 - turn} hours of air left.")  # This makes more sense -before- coordinate input
        print("Input 3 search coordinates")
        d, e, f = [get_input(axis) for axis in "YXZ"]
        print(f"Searching at ({d}, {e}, {f})")  # f-strings are amazing. Also, explain what you're doing instead of
        # dumping variables to the console.
        if a > d:  # If victorious, this won't print anything anyway:
            print("UP")
        elif a < d:
            print("DOWN")
        if b > e:
            print("PORT")
        elif b < e:
            print("STARBOARD")
        if c > f:
            print("FORWARD")
        elif c < f:
            print("BACKWARD")
        # Lets do all program flow changes at the end of the loop instead of halfway.
        if a == d and b == e and c == f:  # Old: A == D and B == E and F == C. Why was the order of F == C mixed up
            # compared to the other comparisons ?
            # Since you just spend an hour searching, you have 1 hour less left. This lets you complete the game with 
            # 0 hours left - just in time.
            print(f"You found the capsule with {4 - turn} hours of air left")  # f-strings! Also, explain what you found.
            return  # Back to main menu. No dedicated function needed.
        if turn == 3:  # python iteration starts at zero, so the last iteration will be 4, and the warning at 3.
            print("Choose your next move carefully... HYPOXIA IMMINENT.")

    print(f"Fail, astronaut dead; capsule was at coordinates ({a}, {b}, {c})")  # f-string. Also, coordinates are 
    # generally given as (1, 2, 3) instead of 1.2.3


def main():
    # Use this function as main menu.
    game()  # Run once, then ask for playing again.
    while True:
        # If we only us a function result once, don't store it in a variable but use it directly. Also, make use of the
        # fact that input() prints a message.
        if input("Play again? Y/N").lower() == "y":
            game()
        else:
            return 0  # Perhaps another script imports this function to play this game. That only works if we don't call
        # sys.exit directly. 


if __name__ == "__main__":
    # This activates the script when we run it.
    sys.exit(main())

Highlights:

  1. Removed 1-line function of winCondition. It didn't serve a purpose.
  2. When quitting a game, it's better to return than call another function. We don't need to remember anything anymore.
  3. Don't nest functions without a really good reason. Good reasons are things like factory functions. If you want them private instead of public, simply prefix them with an underscore.
  4. We don't need new classes like @Anders InvalidInputError. This error exactly matches the builtin ValueError in purpose, so we use that instead. (his variable naming point is good, even if I chose to not include it.)
  5. Separation of concerns. The game() function runs just a single game. The main() (menu) starts games. The get_input() earns it's keep with it's error checking.
  6. Removed end() function. It did the work of the main menu, so that's where it's code went.
\$\endgroup\$
  • 4
    \$\begingroup\$ "In python, code clarity and readability is more important than conciseness." That is almost universally true, not just for Python. \$\endgroup\$ – AlexV Sep 6 at 12:43
  • \$\begingroup\$ Question was about conciseness, so I wanted to give a counterpoint as to why I didn't consider that a high priority. Perhaps I shouldn't restrict it like that, but IMO there's a few reasons Python has it more important than some others - primarily the syntactic meaning of indentation and related concepts. \$\endgroup\$ – Gloweye Sep 6 at 12:48
4
\$\begingroup\$

General structure

You are using a lot of nested functions and recursion. It makes it hard to figure out what is going on just at a glance. I would go with a main function to be a pythonic do-while:

def main()
   while True:
     game()
     if end():
        break

Note that you'll have to modify end() to return a bool, and game() to not call end() but just finnish instead.

Error and input handling

The next thing I would do is change get_input. Its bad to catch all errors - if there would be some unrelated error you would get stuck in an infintie loop.

Using assert to control program flow is also a bit unexpected. As Ben points out in comments, it can be dangerous since asserts can be turned off with a command line options.

One way to do it is to break out the parsing and validation bit into a new function parse_input that returns the integer or throw an exception if the input is invalid:

def get_input(axis):
   while True:
     user_input = input(f"{axis}-Axis: ")
     try:
        return parse_input(user_input)
     except InvalidInputError:
        print("Enter a natural number in the range!")

def parse_input(input_str):
   try:
      input_int = int(input_str)
   except ValueError:
      raise InvalidInputError()
   if input_int > 7:
      raise InvalidInputError()
   return input_int

class InvalidInputError(Exception):
   pass

This code is longer, but I would argue that it is more readable. Opinions may differ.

Variable names

What does A > D mean? I don't know! Something like guess_x > goal_x would be much easier to understand.

A bug!

What happends if the user enters a negative number?

\$\endgroup\$
  • \$\begingroup\$ I just noticed the bottom line of the question! Unfortunately this isn't very helpful when it comes to making things more concice. \$\endgroup\$ – Anders Sep 6 at 10:48
  • 1
    \$\begingroup\$ Using assert to control program flow is actively dangerous in production code; assertions will be ignored if the user calls python with the -O switch. \$\endgroup\$ – Ben Sep 7 at 12:05

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.