Converting Roman numerals to integers and vice versa

def int_to_roman (integer): returnstring='' table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]] 

The element in your list should really be tuples not lists. It should also be a global constant so that you can reuse across both functions.

 for pair in table: 

Use for letter, value in table: rather then indexing the tuples.

 while integer-pair[1]>=0: 

I think the code looks better with spacing around binary operators. Also why this instead of: while integer >= pair[1]:?

 integer-=pair[1] returnstring+=pair[0] 

It'll probably be better to create and append to list and then join the list elements together at the end.

 return returnstring def rom_to_int(string): table=[['M',1000],['CM',900],['D',500],['CD',400],['C',100],['XC',90],['L',50],['XL',40],['X',10],['IX',9],['V',5],['IV',4],['I',1]] returnint=0 for pair in table: continueyes=True 

Whenever I use a logical flag like this, I think: it should be removed. I figure that flags like this only serve to confuse the logic of what you are doing. i think a break is clearer then setting a flag.

 while continueyes: if len(string)>=len(pair[0]): if string[0:len(pair[0])]==pair[0]: 

strings have a funciton: startswith that does this. You should use it here. There is also need to check the length. If you take a slice past the end of a string in python, you just get a shorter string.

 returnint+=pair[1] string=string[len(pair[0]):] else: continueyes=False else: continueyes=False return returnint 

My version of your code:

def int_to_roman (integer): parts = [] for letter, value in TABLE: while value <= integer: integer -= value parts.append(letter) return ''.join(parts) def rom_to_int(string): result = 0 for letter, value in table: while string.startswith(letter): result += value string = string[len(pairs[0]):] return result 

One last thought. Your rom_to_int doesn't handle the case where an invalid string is passed. You might want to consider having it throw an exception or something in that case.

Looks good. I have a few thoughts, every one of them very minor and based in opinion.

• I think it would be clearer to iterate over roms and nums instead of having pairs and having to remember which is which. This uses a Python feature called 'tuple unpacking':

  for (rom, num) in table: print rom print num 

• Concatenating strings over and over is slower than appending to a list - but that this is something that would likely never matter for this application! If you want, you could collect your Roman numerals in a list before joining them at the end:

  l = [] for i in range(10): l.append('s') s = "".join(l) print s 

• table is information common to both functions; not that it's going to change, but if evidence for new Roman numerals ever was found, it'd be nice to just add them in one place. table could therefore be a module-level variable.

• I personally find continueyes to be an awkward variable name - you could use continue_, following a convention of adding a trailing underscore to avoid a Python keyword.

• You could use break instead of setting continueyet = True and waiting for the while to check the condition.

  while True: if done: break