-2

I'm writing a simple game in Python where the user enters a matrix element defined by row and column separated by a space. I want to validate this input.

I think I have solved it correctly with the code below, but I'm curious if my approach is a good one, or if there are obvious improvements I could make or errors in my logic please.

player_choice = ["",""]
while True:
    player_choice = input("Enter the row and column of your choice separated by a space: ").split(" ")
    if len(player_choice) != 2:
        continue
    if not player_choice[0].isdigit() or not player_choice[1].isdigit():
        continue
    if int(player_choice[0]) <= 0 or int(player_choice[0]) > ROWS:
        continue
    if int(player_choice[1]) <= 0 or int(player_choice[1]) > COLUMNS:
        continue
    else:
        break
martineau
  • 99,260
  • 22
  • 139
  • 249
Robin Andrews
  • 2,407
  • 4
  • 27
  • 62

1 Answers1

1
while True:

This is never great and should indicate to you that there is a better way to design this code. Even if it is just using boolean flag.

if len(player_choice) != 2:
    continue
if not player_choice[0].isdigit() or not player_choice[0].isdigit():

So aside from the obvious typo where the second clause should have been player_choice[1], in python it is more idiomatic to try instead of if (https://devblogs.microsoft.com/python/idiomatic-python-eafp-versus-lbyl/). Also, consider providing some user feedback as to (a) the fact that the command failed and (b) why it failed:

try:
    row = int(player_choice[0])
    col = int(player_choice[1])
except ValueError:
    print(f"Input must be two numbers, however non-digit characters were received."
except IndexError:
    print("The input should be two numbers separated by a space but no space was entered")

For validating the limits, again consider offering some feedback. Also ROWS etc are not such descriptive names. num_rows is better. Also, instead of a constant, rather make this whole thing a function and set these as default arguments instead;

def validate_user_input(player_choice: str, num_rows: int = 10, num_cols: int = 10) -> bool:
    try:
        row, col = player_choice.split()
    except ValueError:
        print("Bad input: The input should be exactly two numbers separated by a space.")
        return False
    try:
        row = int(row)
        col = int(col)
    except ValueError:
        print(f"Input must be two numbers, however non-digit characters were received."
        return False

    if row < 0 or row > num_rows:
        print(f"The first number must be between 0 and {num_rows} but {row} was passed.")
        return False
    if col < 0 or col > num_rows:
        print(f"The second number must be between 0 and {num_cols} but {col} was passed.")
        return False
    return true

And then your loop becomes:

valid_input = False
while not valid_input:
    player_choice = input("Enter the row and column of your choice separated by a space: ")
    valid_input = validate_user_input(player_choice)
Dan
  • 43,452
  • 14
  • 75
  • 140
  • 1
    `if row < 0 or row > num_rows` => `if row not in range(num_rows)` ? – 301_Moved_Permanently Jul 11 '19 at 13:47
  • @MathiasEttinger I guess it's less efficient as it has a linear runtime compared to constant runtime and also it would be `range(num_rows + 1)` so I'm sure it's worth it to be honest. – Dan Jul 11 '19 at 13:48
  • @MathiasEttinger Although you could do this I guess: `if not 0 < row < num_rows:` – Dan Jul 11 '19 at 13:55
  • 2
    [contains check in range objects are O(1) in Python 3](https://stackoverflow.com/q/30081275/5069029). But sure, having it be `num_rows + 1` removes a bit of purity... – 301_Moved_Permanently Jul 11 '19 at 13:55
  • @MathiasEttinger oh that's interesting, thanks for the link – Dan Jul 11 '19 at 13:56
  • `if not (0 <= row <= num_rows)` then, yes, I originaly contemplated that but found `range` to express the same thought in a "closer to plain-english" manner. – 301_Moved_Permanently Jul 11 '19 at 13:57