14
\$\begingroup\$
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]]

    for pair in table:

        while integer-pair[1]>=0:

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

    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

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

                if string[0:len(pair[0])]==pair[0]:
                    returnint+=pair[1]
                    string=string[len(pair[0]):]

                else: continueyes=False
            else: continueyes=False

    return returnint
\$\endgroup\$
8
\$\begingroup\$
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.

\$\endgroup\$
  • 1
    \$\begingroup\$ thank you! This is incredibly helpful. This all makes a lot of sense. I learned python (my first prog. language since turbo pascal in 1980s) very quickly and with specific tasks in mind, really don't yet know how to utilize its features... \$\endgroup\$ – Anthony Curtis Adler Oct 3 '11 at 23:09
5
\$\begingroup\$

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