1
\$\begingroup\$

I submitted the following Python 3 code for this HackerRank challenge. Given an n × n grid with an m and a p in random cells of the grid, it prints the next step in a path that moves the m to the p. I'd like any feedback on this.

def nextMove(n,r,c,grid):
    #First, find the princess
    for i in range(len(grid)):
        for j in range(len(grid[0])):
            if grid[i][j] == "p":
                a = i
                b = j    
    # i = row, j = column
    nm = ("UP" if (r-a) > 0 else "DOWN")
    if r-a == 0:
        nm = ("LEFT" if (c-b) > 0 else "RIGHT")
    return nm

n = int(input())
r,c = [int(i) for i in input().strip().split()]
grid = []
for i in range(0, n):
    grid.append(input())

print(nextMove(n,r,c,grid))
\$\endgroup\$
3
\$\begingroup\$

Mathias gave an excellent answer. However, the problem gave some starting "suggestions", like working with the whole grid. If we are to stick to that, I suggest you make a separate function for finding the princess:

def findPrincess(grid):
    for r, line in enumerate(grid):
        if "p" in line:
            return (r, line.index("p"))

This way, the function stops the moment it finds "p", instead of going through the rest of the lines, like in your code.

Also, we avoid (fairly slow) indexing by using enumerate, which is almost always better than range(len(...)).

I'll also add that Mathias used exception handling where I chose if. There is some cost to exception handlers whenever an exception occurs, but there is also the fact that my code looks twice for "p" in the last line. On average, I have no idea which is faster, nor do I think it matters for small grids. In the end, I picked if a) to show an alternative approach, and b) because it looked visually a bit nicer to me.

As for the rest of your function, for the sake of readability, I'd refactor

nm = ("UP" if (r-a) > 0 else "DOWN")
if r-a == 0:
    nm = ("LEFT" if (c-b) > 0 else "RIGHT")
return nm

like this:

if r > a:
    return "UP"
elif r < a:
    return "DOWN"
elif c > b:
    return "LEFT"
elif c < b:
    return "RIGHT"
else:
    return "ALREADY THERE"

The last elif...else... block can be replaced with else: return "RIGHT", but you'll need this version soon, for the cleaning bot problems.

\$\endgroup\$
  • 1
    \$\begingroup\$ EAFP (exception handling) is the way I usually favors in Python. But I agree that, in this case, where all tests will fail except the last one, it should be faster to test with ifs instead. \$\endgroup\$ – 409_Conflict Jun 12 '16 at 13:09
4
\$\begingroup\$

Parsing input

input().strip().split() can be reduced to input().split() as split without parameters has extra logic to remove empty strings in the result. It feels also more idiomatic to unpack the two integers using r, c = map(int, input().split())

Parsing princess position

First of, you could create the grid using a list-comprehension rather than append:

grid = [input() for _ in range(n)]

Note that I dropped 0 from the range parameter as it the default starting point.

But you don't even need to build the whole grid since you’re only interested in knowing the princess position. You could search for it while parsing the input:

def princess_position(grid_size):
    for row in range(grid_size):
        try:
            return row, input().index('p')
        except ValueError:
            # Princess not on this line
            pass
    # Princess not in grid
    return -1, -1

grid_size = int(input())
bot_row, bot_column = map(int, input().split())
princess_row, princess_column = princess_position(grid_size)
print(next_move(bot_row, bot_column, princess_row, princess_column))

Comparing positions

You don't need to put parenthesis around ternary operators. You can also simplify a - b > 0 to a > b; same for other comparisons. Last thing, you can perform less work by comparing a == r first and returning left or right before comparing for up and down:

def next_move(bot_row, bot_column, princess_row, princess_column):
    if bot_row == princess_row:
        return 'LEFT' if bot_column > princess_column else 'RIGHT'
    return 'UP' if bot_row < princess_row else 'DOWN'

Top-level code

It is better practice to wrap your top-level code under an if __name__ == '__main__' statement, as it allows you to import your file without executing the whole thing:

def next_move(bot_row, bot_column, princess_row, princess_column):
    if bot_row == princess_row:
        return 'LEFT' if bot_column > princess_column else 'RIGHT'
    return 'UP' if bot_row < princess_row else 'DOWN'

def princess_position(grid_size):
    for row in range(grid_size):
        try:
            column = input().index('p')
        except ValueError:
            # Princess not on this line
            pass
        else:
            return row, column
    # Princess not in grid
    return -1, -1

if __name__ == '__main__':
    grid_size = int(input())
    print(next_move(
        *map(int, input().split())
        *princess_position(grid_size))

I also changed the way to use unpacking to simplify variable managements.

\$\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.