Encryption using a mirror field

Question

This is my attempt at the mirror encryption challenge posted on r/dailyprogrammer/. The idea is that each character is to be replaced by its counterpart in the mirror field, as if a laser pointer were reflected off of all of the diagonal barriers in the grid.

Since I am relatively new to python, I'd like for someone to just have a look and point out some things that I need to look out for and where I can improve please.

class Encrypt(object):
 """
 String encryption using a 13x13 mirror field
 """
 mirrorField = (
 ('', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', ''),
 ('A', '', '', '', '\\', '\\', '', '', '/', '\\', '', '', '', '', 'n'),
 ('B', '', '', '', '', '', '', '', '', '', '', '', '', '\\', 'o'),
 ('C', '', '', '', '/', '', '', '', '', '', '', '', '', '', 'p'),
 ('D', '', '', '', '', '', '', '\\', '', '', '', '', '', '\\', 'q'),
 ('E', '', '', '', '', '\\', '', '', '', '', '', '', '', '', 'r'),
 ('F', '', '', '/', '', '', '', '', '', '', '/', '', '', '', 's'),
 ('G', '\\', '', '', '/', '', '', '', '', '', '', '\\', '', '', 't'),
 ('H', '', '', '', '', '', '\\', '', '', '', '', '', '', '', 'u'),
 ('I', '\\', '/', '', '', '', '', '', '', '', '', '', '', '', 'v'),
 ('J', '/', '', '', '', '', '', '', '', '', '', '', '', '', 'w'),
 ('K', '', '', '', '', '', '', '', '', '', '', '\\', '', '', 'x'),
 ('L', '', '', '', '', '\\', '/', '', '', '', '', '', '', '', 'y'),
 ('M', '', '', '', '/', '', '', '', '', '', '', '', '/', '', 'z'),
 ('', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '')
 )
 def __init__(self):
 self.direction = ''
 self.encrypted = ''
 self.running = False
 def start(self, string):
 """
 Starting point for encryption
 Args:
 string (string): String to be encrypted/decrypted
 """
 for char in string:
 y, x = self.find_char_in_mirror(char)
 self.running = True
 # Set direction based on the starting position
 if y == 0:
 self.direction = 'down'
 elif y == 14:
 self.direction = 'up'
 elif x == 14:
 self.direction = 'left'
 else:
 self.direction = 'right'
 self.encrypt(y, x)
 print self.encrypted
 def encrypt(self, y, x):
 """
 Main function for checking direction and updating positions
 Args:
 y (int): y position in the mirrorField
 x (int): x position in the mirrorField
 """
 while(self.running):
 if self.direction == 'left':
 x -= 1
 self.check_mirror_position(y, x, 'down', 'up')
 if self.direction == 'right':
 x += 1
 self.check_mirror_position(y, x, 'up', 'down')
 if self.direction == 'up':
 y -= 1
 self.check_mirror_position(y, x, 'right', 'left')
 if self.direction == 'down':
 y += 1
 self.check_mirror_position(y, x, 'left', 'right')
 def check_mirror_position(self, y, x, fm_direction, bm_direction):
 """
 Checks the corresponding positions for existing mirrors and adjusts the direction accordingly
 Args:
 y (int): y position in the mirrorField
 x (int): x position in the mirrorField
 fm_direction (string): Updated direction if a / mirror is found
 bm_direction (string): Updated direction if a \\ mirror is found
 """
 if Encrypt.mirrorField[y][x] == '/':
 self.direction = fm_direction
 elif Encrypt.mirrorField[y][x] == '\\':
 self.direction = bm_direction
 elif Encrypt.mirrorField[y][x] != '':
 self.running = False
 self.encrypted += Encrypt.mirrorField[y][x]
 else:
 pass
 def find_char_in_mirror(self, char):
 """
 Find the x and y position of a character in the mirrorField
 Args:
 char (string): character to be searched for in mirrorField
 """
 for y, row in enumerate(Encrypt.mirrorField):
 for x, col in enumerate(Encrypt.mirrorField[y]):
 if Encrypt.mirrorField[y][x] == char:
 return y, x
encrypt = Encrypt()
encrypt.start('TpnQSjdmZdpoohd')

Answer 1

Your execution flow is very convoluted and unconventional. (The technical term for that is "spaghetti".) At first glance, it appears that you have defined a useful class and methods. However, on closer inspection, those "functions" are communicating with each other through variables (direction, encrypted, and running) that are somewhat global in nature.

Let's consider the instance variables.

encrypted is cleared in __init__(), built in check_mirror_position(), and output in start(). What if you want to encrypt another string using the same mirror field — what happens if you call start() again?

direction is initialized in __init__() (unnecessarily), reinitialized in start(), consulted in encrypt(), and reassigned in check_mirror_position().

running is initialized to False in __init__() (unnecessarily), reinitialized to True in start(), and tested in encrypt(), until it is set to False in check_mirror_position().

---

Consider the main code:

encrypt = Encrypt()
encrypt.start('TpnQSjdmZdpoohd')

What does "start" do? If there is a start, where does it finish? (Actually, start() does all of the work from start to finish!) Does it print anything, and if so, where is the print statement? What if you wanted to write unit tests or incorporate the encryption routine into a web application? You couldn't easily do it, since it's hard-wired to print its output to stdout. A well designed class would be used more like this:

encryptor = MirrorFieldEncryptor(grid)
print(encryptor.encrypt('TpnQSjdmZdpoohd'))

As you can see, I would have expected that the encrypt() method would be the one to call to encrypt a string. But that's not what your encrypt() method does. Rather, it accepts a (y, x) position. Huh? Encrypting a string or a character makes sense, but what does it mean to encrypt a position?

check_mirror_position() is unintuitively named, since I would expect a method named check_something to validate rather than build the result.

find_char_in_mirror() makes reasonable sense — it is the only method that works by accepting a parameter and returning its result, with no side-effects! It's not very efficient, though — it potentially scans the entire mirrorField to find the character. Also, since it could be considered a "private" method, it would be conventional to name it _find_char_in_mirror().

---

What should be done to reorganize the code?

• Rename start() to encrypt().
• Rename private methods to more meaningful names, with a leading underscore.
• Change all instance variables to local variables.
• Change mirrorField from a static variable to an instance variable, and rename it mirror_field to comply with PEP 8.
• Change all methods to return useful values.

class MirrorEncrypt(object):
 """
 String encryption using a 13x13 mirror field
 """
 def __init__(self):
 # TODO: read mirror_field from somewhere
 self.mirror_field = (...)
 def encrypt(self, string):
 """Encrypt the string with the mirror field as the key, returning the
 encrypted string."""
 encrypted = ''
 for char in string:
 y, x = self._find_char_in_mirror(char)
 # Set direction based on the starting position
 direction = ('down' if y == 0 else
 'up' if y == 14 else
 'left' if x == 14 else
 'right')
 encrypted += self._follow(y, x, direction)
 return encrypted
 def _follow(self, y, x, direction):
 """Return the resulting character from tracing the beam from the given
 position and direction."""
 while True:
 if direction == 'left':
 x -= 1
 direction = self._turn(y, x, direction, 'down', 'up')
 if direction == 'right':
 x += 1
 direction = self._turn(y, x, direction, 'up', 'down')
 if direction == 'up':
 y -= 1
 direction = self._turn(y, x, direction, 'right', 'left')
 if direction == 'down':
 y += 1
 direction = self._turn(y, x, direction, 'left', 'right')
 if self.mirror_field[y][x] and self.mirror_field[y][x] not in '\\/':
 return self.mirror_field[y][x]
 def _turn(self, y, x, direction, direction_slash, direction_backslash):
 """Determine the new direction of the beam."""
 if self.mirror_field[y][x] == '/':
 return direction_slash
 elif self.mirror_field[y][x] == '\\':
 return direction_backslash
 else:
 return direction
 def _find_char_in_mirror(self, char):
 """..."""
 for y, row in enumerate(self.mirror_field):
 for x, col in enumerate(self.mirror_field[y]):
 if self.mirror_field[y][x] == char:
 return y, x
encrypt = MirrorEncrypt()
print(encrypt.encrypt('TpnQSjdmZdpoohd'))

There is more that could be done. For example, I would analyze the grid to produce a character-to-character mapping, then the text can be encrypted with a simple lookup for each character. (See this question for how I would do it.)

Answer 2

Without analysing your algorithm, I would change a few things in how the code is arranged.

First, your input is wrong, as the problem states that you only get \13ドル \times 13\$ grid (no letters on the sides of the grid, and your empty strings would become single-space strings). Related to that, the mirror can be given as a list or a tuple of strings, i.e.,

mirrorField = [
 r" \\ /\ ",
 r" \",
 r" / ",
 r" \ \",
 r" \ ",
 r" / / ",
 r"\ / \ ",
 r" \ ",
 r"\/ ",
 r"/ ",
 r" \ ",
 r" \/ ",
 r" / / ",
]

Second, since you're making a class, I'd make a function set_mirror that would accept a mirror as the above list or a single string with newlines, and make a dictionary later used by encrypt() to map the letters. There is no need to translate every letter of the text by parsing the mirror, because that could be slow and lengthy for bigger texts.

Third, using strings "up", "down", etc. is hardly practical. You can use a single variable direction that can be a tuple of two integers, each of them -1, 0, or 1. For example, (0, -1) would mean "up" because (x+0, y+(-1)) is "up" from (x, y). Then your move doesn't need if's. Instead, you can have (x, y) = (x+dir[0], y+dir[1]) or something similar.

I think it would also be a good idea to use raw string literals (r"...") instead of ordinary ones, as I did above, to avoid the need for escaping your hard-coded backslashes.

• 200_success: Raw strings sound like an appealing idea at first, but they turn out to be not that useful at all, because raw strings aren't completely raw! In particular, the lines that end with a backslash are illegal. In the end, the raw strings create more confusion than benefit.
• Vedran Šego: You are right, but given that the grid has fixed size \13ドル \times 13\,ドル one can always add an additional space (or any other character) that wouldn't affect the algorithm, leaving the hardcoded grid easier to view than if each backslash mirror was escaped.
• 200_success: Raw strings are weird, though. See what happens with the two consecutive backslashes on the first row.
• Vedran Šego: 'r" \\ /\ " -> ' \\\\ /\\ ' They remain as 2 backslashes. Why is that a problem?
• 200_success: My mistake. I was playing around with raw strings embedded inside a docstring, and got confused.