4
\$\begingroup\$

I am having the following problem. My code works, but it seems like it can improve a lot. I am fairly new to python and can't seem to get this block any better. The logic behind it goes like this.

I have a dictionary that acts like a tree, it looks like this:

SampleTree[10000000] = ['ACC','ARRAY']
SampleTree[10010000] = ['ACC_ABO','ABO']
SampleTree[10020000] = ['ACC_CHILDLOCK','OPTION']
SampleTree[10030000] = ['ACC_RECO','ARRAY']
SampleTree[10030100] = ['ACC_RECO_ACTIVATE']
SampleTree[10030200] = ['ACC_RECO_ACTIVATE']
SampleTree[10030201] = ['ACC_RECO_ACTIVATE']
SampleTree[10030202] = ['ACC_RECO_ACTIVATE']
SampleTree[10040000] = ['ACC_ADDUSER','OPTION']
SampleTree[10050000] = ['ACC_SETTINGS','ARRAY']

Where the dict key acts like a unique key, which I use to recognize in my program. I use a for loop to iterate over the full dict like this:

KEYLIST = SampleTree.keys()
KEYLIST.sort()

for KEY in KEYLIST :
    if (KEY % 1000000) == 0 :
        BACKS = 0
    elif (KEY % 1000000) == 10000 and (LAST % 1000000) == 0 :
        BACKS = 1
    elif (KEY % 10000) == 0 and (LAST % 1000000) == 10000 :
        BACKS = 2
    elif (KEY % 10000) == 100 and (LAST % 10000) == 0 :
        BACKS = 1
    elif (KEY % 10000) == 0 and (LAST % 10000) == 0 :
        BACKS = 2
    elif (KEY % 10000) == 0 and ((LAST % 10000) % 100) == 0 :
        BACKS = 3
    elif (KEY % 10000) == 0 and (LAST % 100) != 0 :
        BACKS = 4
    elif (KEY % 100) == 1 and ((LAST % 10000) % 100) == 0 :
        BACKS = 1
    elif (KEY % 100) == 0 and (LAST % 100) == 0 :
        BACKS = 2
    elif (KEY % 100) == 0 and (LAST % 100) != 0 :
        BACKS = 3
    elif (KEY % 100) != 0 and (LAST % 100) != 0 :
        BACKS = 2

    LAST=KEY

As you can see in the code, it uses a bunch of if elif tests to see in which branch the Key currently is. And accordingly it sets the BACKS variable how it should be. After this code there are a bunch of operation based on the KEY value and on the BACKS variable.

The BACKS variable depends on the transition of each KEY. Some examples:

from : 10 00 00 00 00 to : 10 01 00 00 00 you need to go BACK 1 time

from : 10 01 00 00 00 to : 10 01 01 00 00 you need to go BACK 1 time

from : 10 01 01 00 00 to : 10 01 02 00 00 you need to go BACK 2 times

from : 10 01 02 00 00 to : 10 02 00 00 00 you need to go BACK 3 times

So what I am trying to say is that there doesn't seem to be a particular logic behind all this. Can you please help me?

My whole problem with the code is that it is so ugly and look like it can improve a lot. But I just can't see a better way. Another big issue is expansion. If I for example add another layer to the tree (add two more zeros to the KEY), the whole logic falls apart.

\$\endgroup\$
  • 2
    \$\begingroup\$ Seems like you're trying to implement a state machine. Give this (stackoverflow.com/questions/2101961/python-state-machine-design) a close read for ideas on how to handle expansion in a cleaner way. \$\endgroup\$ – shivsky Sep 16 '14 at 11:23
  • \$\begingroup\$ Seems like overcompensation to me, I know what your going for, but I can't seem to make it fit, especially with expansion in mind. Is there a way to strip zeros from integers and to get the number of stripped numbers? \$\endgroup\$ – Martijn Claes Sep 16 '14 at 12:55
  • 3
    \$\begingroup\$ I do not undderstand the logic of how BACK is calculated, can you elaborate a bit? \$\endgroup\$ – Jaime Sep 16 '14 at 13:44
  • \$\begingroup\$ Well the logic part was the root of my problem, by having another go at it, I managed to rewrite the code by finding a proper alogrithm inside the BACK calculation. I posted the answer. \$\endgroup\$ – Martijn Claes Sep 17 '14 at 9:08
3
\$\begingroup\$

You've got tree paths encoded as integers such as: 10030201. Your code would much easier to write if instead they tuples like: (10, 03, 02, 01). If at all possible, you should change your dictionary to always use tuples. If not possible, have a function to convert the integers to tuples when you need them. If you do that, you'll find its fairly easy to devise a general implementation of your BACKS logic.

On the stylistic front: We typically use ALL_CAPS only for constants. We use lowercase_with_underscores for local variables which all of your stuff appears to be.

My solution:

def calculate_backs(source, destination):
    """
    Return how many "resets" are required to transform source into destination
    """

    def as_tuple(id_number):
        """
        Converts the id_number into a path sequence: 
        10010200 becomes (10, 1, 2)
        """
        pieces = []
        while id_number:
          pieces.append(id_number % 100)
          id_number /= 100
        pieces.reverse()

        # position 0 denotes a root node in the tree, so remove any trailing
        # zeros.
        while pieces[-1] == 0:
          pieces.pop()
        return tuple(pieces)

    def calculate(source, destination):
        if source and destination and source[0] == destination[0]:
            # If both source and destination agree for the first piece of the
            # path, drop that piece and determine the number of resets required
            # for the rest.
            return calculate(source[1:], destination[1:])
        else:
            # If the path disagrees on the first piece, we have to reset 
            # everything remaining in both paths.
            return len(source) + len(destination)

    return calculate(as_tuple(source), as_tuple(destination))
\$\endgroup\$
  • \$\begingroup\$ Thank you for the style tips, I will keep them in mind. I have another solution for my problem, I posted the answer here. Working with tuples is out of my control. The integer Keys are already in place and they don't really want to change it. \$\endgroup\$ – Martijn Claes Sep 17 '14 at 9:09
  • \$\begingroup\$ @MartijnClaes, you don't have to convert all the code to use tuples for it to be useful. You can locally convert to a tuple where it helps. See my edit. \$\endgroup\$ – Winston Ewert Sep 17 '14 at 13:20
1
\$\begingroup\$

First of all thank you for all the anwsers. I took another go at finding a relationship between all the numbers and i found one. This is what i came up with in code. As u can see it is a lot simplistic and it can handle expansions. I tried to use the python style more with the var names. Any feedback is apreciated.

KEYSTRING = str(KEY)
deleted = 0
stripped = KEYSTRING.rstrip('0')
deleted += len(KEYSTRING) - len(stripped)
if deleted > max_length :
    max_length = deleted
if deleted == max_length :
    backs = 0
else :
    if (stripped[-1:] == '1') and (deleted % 2 == 0) :
        backs = 1
    elif deleted > last :
        if len(str(KEY))%2==0 :
            backs = len(str(KEY))/2
            backs -= last/2
        else :
            backs = (len(str(KEY))+1)/2
            backs -= last/2
    else :
        backs = 2
\$\endgroup\$
  • \$\begingroup\$ Good job! The biggest issue here was to find a relationship between the numbers. You better then anyone could do that, that is because you know the problem better and you have spent more time to think about it. Nice Job! \$\endgroup\$ – Bruno Costa Sep 17 '14 at 9:16
  • \$\begingroup\$ That is true, took me a while to get it. Couple of papers filled with numbers and a headache later, I came up with this. \$\endgroup\$ – Martijn Claes Sep 17 '14 at 10:02
  • \$\begingroup\$ You refer to last and max_length but its not clear how these get assigned. You are still using ALL_CAPS for KEYSTRING and KEY. You have spaces before your colons which isn't standard style. You call str(KEY) several times despite having stored that in KEYSTRING. \$\endgroup\$ – Winston Ewert Sep 17 '14 at 13:20

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.