Comparing Server Version Number History

Question

I have this code that compares all version numbers of servers. (The version numbers are stored in an XML file) Before the code would compare the current version number with the previous. This caused some problems because in the cluster of servers, one has a different version number than the rest. If my code happened to connect to that server, it would think it was a change. So then I made it compare the three previous version numbers. The same issue was happening. So I got frustrated and made it load all the version numbers in a list and check the current one against the list. Will this cause a huge performance decrease (the list would be 50 elements at most)? Is there a better way?

#Loads all version numbers in list,
#starts at index 1 because the the first
#element in the XML file is has version number of 0

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(historyLength):
        if i >= 1:
            l.append(hist[len(hist)-i].get("version"))
    return l

#Checks the current version number to each element
#in the list to see if it is the same. If there is a
#match then the it is not a newer version number,
#otherwise it is a new version number and the function
#returns True.

def checkAllVersions(self, currentVersion, allHist):
    historyLength = len(allHist)
    for i in range(historyLength):
        if(allHist[i] == currentVersion):
            return False
    return True

def compareVersions(self,parent,componentTarget, write):
    componentsTree = parent.findall("component")
    currentVersion = self.componentVersions[self.components.index(componentTarget)]

    for comp in componentsTree:
        if(comp.get("id") == componentTarget):
            hist = comp.findall("history")
            allHist = self.loadAllVersions(hist)
            isNewVersion = self.checkAllVersions(currentVersion, allHist)
            if isNewVersion:
                if write and currentVersion != None and self.getComponentURL(componentTarget) != None:
                        event = ET.SubElement(comp,"history")
                        event.set("time",str(int(time.time())))
                        event.set("version",currentVersion)
                        event.set("sourceUrl",self.getComponentURL(componentTarget))
                        event.set("pretty",self.componentsPretty[self.components.index(componentTarget)])
                return True
    return False

Answer 1

Before anything, I reckon you should have a look at PEP 8 which is the usual Style Guide for Python Code. Among other things, your names for variables and functions does not follow the lowercase_with_words_separated_by_underscores convention.

---

In :

def checkAllVersions(self, currentVersion, allHist):
    historyLength = len(allHist)
    for i in range(historyLength):
        if(allHist[i] == currentVersion):
            return False
    return True

this does not correspond to the pythonic way of looping over a container. There's a cleaner way that doesn't involve indices.

def checkAllVersions(self, currentVersion, allHist):
    for e in allHist:
        if(e == currentVersion):
            return False
    return True

As an additional tip, there's a cool way to write this using all or any :

def checkAllVersions(self, currentVersion, allHist):
    return all(e != current version for e in allHist)

Edit:

And there is an even more obvious way to write it and I completely missed it :

def checkAllVersions(self, currentVersion, allHist):
    return currentVersion not in allHist

---

In :

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(historyLength):
        if i >= 1:
            l.append(hist[len(hist)-i].get("version"))
    return l

instead of iterating from 0 to historyLength-1 and then consider only values i such that i >= 1, you could just iterate from 1 to historyLength-1. Just use range(1,historyLength).

def loadAllVersions(self, hist):
    historyLength = len(hist)
    l =[]
    for i in range(1,historyLength):
        l.append(hist[len(hist)-i].get("version"))
    return l

However, it seems like something even smarter could be done here.

At the end of the day, my assumption is that what you want to do is just to iterate over hist from the beginning to the end and call .get("version") on each element. This is not what you are currently doing because you are not processing the first element of the list (index is 0 and would correspond to i = historyLength).

You have more than one way to do it but the most straight-forward solution involves looping over your list in reversed order with reversed :

def loadAllVersions(self, hist):
    l =[]
    for e in reversed(hist):
        l.append(e.get("version"))
    return l

and then, this calls for list comprehension :

def loadAllVersions(self, hist):
    return [e.get("version") for e in reversed(hist)]

---

It is now time to have a look at def compareVersions(self,parent,componentTarget, write), a much bigger chunk.

First thing I'd like to point out, you do no follow this part of PEP 8 :

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

I'd like you to confirm my assumption about loadAllVersions before going any further.

Answer 2

Josay's answer is excellent. Just wanted to add one more detail:

In your code, you use large blocks of comments above your functions to describe what they do. You should consider using docstrings instead.

So, for example, your first function would look something like...

def loadAllVersions(self, hist):
    """
    Load all version numbers into a list.  Begin at
    index 1 to skip the first line in the XML file,
    which always has a version number of 0.

    Arguments:
        hist: list of version XML entries

    Returns:
        a list of version numbers
    """
    historyLength = len(hist)
    l =[]
    for i in range(historyLength):
        if i >= 1:
            l.append(hist[len(hist)-i].get("version"))
    return l