5
\$\begingroup\$

I submitted the following Python 3 code for this HackerRank challenge. Given an n × n grid with an m in the center and a p in one corner, it prints the directions to move the m to the p.

I wanted to know how improve it or other observations.

#!/usr/bin/python
def displayPathtoPrincess(n,grid):
    #first, find where p is located 
    flag = True
    n = n-1
    while(flag):
        if grid[0][n] == 'p':
            flag = False 
            a = 0
            b = n
        if grid[n][0] == 'p':
            flag = False 
            a = n
            b = 0
        if grid[0][0] == 'p':
            flag = False 
            a = 0
            b = 0 
        if grid[n][n] == 'p':
            flag = False 
            a = n
            b = n 
        #else:
            #print("Something broke?") #Why does this execute?
        y = a - int(n/2)
        x = b - int(n/2)    
        while 0 != y:
            if y < 0:
                y = y+1
                print("UP")
            if y > 0:
                y = y-1
                print("DOWN")
        while 0 != x:
            if x <0:
                x = x+1
                print("LEFT")
            if x >0:
                x = x-1
                print("RIGHT")



m = int(input())
grid = [] 
for i in range(0, m): 
    grid.append(input().strip())

displayPathtoPrincess(m,grid)
\$\endgroup\$
7
\$\begingroup\$

The function should be path_to_princess(grid), using the naming convention specified in PEP 8. The n parameter is redundant, as it can be inferred from the grid's dimensions. In any case, redefining n=n-1 is confusing. Also, it would be better to leave all I/O out of the function, so that it just does computation.

The while(flag) loop shouldn't exist. If the princess is in another castle, you'll end up with an infinite loop; otherwise, the code executes just once.

Use the // operator for integer division.

Explicit looping can be greatly reduced through liberal use of list comprehensions, generators, and list multiplication.

def path_to_princess(grid):
    z = len(grid) - 1
    corners = [
        (0, 0), (0, z),
        (z, 0), (z, z)
    ]
    p = next((r, c) for r, c in corners if grid[r][c] == 'p')
    m = (z // 2, z // 2)
    row_diff, col_diff = p[0] - m[0], p[1] - m[1]    
    yield from (['UP'] * -row_diff if row_diff < 0 else ['DOWN'] * row_diff)
    yield from (['LEFT'] * -col_diff if col_diff < 0 else ['RIGHT'] * col_diff)

n = int(input())
grid = [input().strip() for _ in range(n)]
for move in path_to_princess(grid):
    print(move)
\$\endgroup\$
  • 3
    \$\begingroup\$ While I do agree with your answer, the function's name and arguments are a property of the problem and not a solution. \$\endgroup\$ – Vedran Šego Jun 10 '16 at 22:14
  • \$\begingroup\$ @VedranŠego Actually, it looks like the HackerRank template is merely a suggestion, and can be replaced by any code that works with the specified input/output format. \$\endgroup\$ – 200_success Jun 10 '16 at 22:17
  • \$\begingroup\$ It seems so, yes. However, these problems are not that consistent. IIRC, some of the problems in that same group have if __name__ == "__main__": which may or may not mean that the code is used as a module. IMO, best not mess with what is given, even if it is PEP8 inconsistent. Otherwise, I'd also consider loading the grid with a list comprehension grid = [input().strip() for _ in range(m)]. \$\endgroup\$ – Vedran Šego Jun 10 '16 at 22:23
4
\$\begingroup\$

I would start by losing flag. All it does is keep looping if the princess isn't found. Does this really make sense?

Consider consolidating your logic and using the built in print functionality. In pseudo code, you might have something like this:

horizontalDistance = princess Location X- start position
verticalDistance = princess location Y - start position

You can then use the print functionality to print a string multiple times using the absolute value of the distance. Something like this:

print(("LEFT" if horizontalDistance > 0 else "RIGHT") * abs(horizontalDistance))

You could obviously use a variable, rather than the ternary operator for readability.

\$\endgroup\$
1
\$\begingroup\$
  1. Remove the debugging code (else how did I get here)
  2. This should have been 2 functions, the first is find the princess. The second is print the path to the princess.
\$\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.