4
\$\begingroup\$

I have code that backup files to remote servers.

There are 2 pandas data frames:

  • storedfiles: that points to the files already stored in previous runs
  • filelist: a list of candidates to be stored in one of the depots (comes from a config file).

    Filelist structure has the following columns:

    In [10]: filelist
    Out[10]: 
                     Name     Objects    Class Subclass Creator  \  
    0  /backups/dir001/test00         400    waves     time   john      
    1  /backups/dir002/test00         400    waves     time   john      
    
                  Created  Datahost Res  Total Size  
    0  12-Feb-15 10:10:59       NaN   D     1609728  
    1  04-Jan-15 14:40:38       NaN   D     1609728
    

To evaluate the storage I check the version on filelist against the version on storedfiles. If the version on file list is newer, it will be stored in some depot (the old version will be deleted). (edit) But to get the "version" I "need" to fetch it from a XML structure. Using an class to read the XML and process the data on it.

# 'a' is a instance of a internal XML processing class

# This fetchs the XML structure related to the file 'f' from filelist.
# Its a type(xmlfile) is 'str'
xmlfile = a.get_xml(f)

# This is a object to the structured data loaded on xmlfile.
# type(xmldata) is aXML
xmldata = a.load(xmlfile)

# With that I can fetch informations from my file as:
# That returns an datetime.datetime structure
type(xmldata.get_data_created())
<type 'datetime.datetime'>

# As I do at: 
version = int(xmldata.get_data_created().strftime("%s"))

To control versions I get the Created field (that stores a datetime) and convert it to a Unix timestamp.

My main concern is that I use too many ifs. I feel the code is ugly and I wish have hints to do a neater and prettier code.

# Stored files is in fact a local csv file with this format
# It holds all file names that has being processed and stored in "depot" server
storedfiles= """
Filename,Version,Depot
foo.txt,12342412,server1
bar.mov,14144862,server2
tmp.log,13327702,server1
"""

cfcolumns=['Filename','Version','Depot']

# stdf (stored files dataframe)
# Is the dataframe that maps the local csv file with processed files
sfdf= panda.Dataframe(storedfiles,...)

# filelist is a pandas dataframe that holds a list of files to be archived
filelist = pandas.DataFrame(...)

# If there is some candidate to archive
if len(filelist.index)!=0:

    for f in filelist.filename:
        # To fetch the attributes I need to process some XML data...
        xmlfile = a.get_xml(f)
        xmldata = a.load(xmlfile)

        # Use data unix timestamp as version
        version = int(xmldata.get_data_created().strftime("%s"))

        # Get the panda series that match the 'filename' with f in loop
        # That is, look if the filename was already backuped.
        ssts = sfdf.loc[sfdf['filename'] == f]

        # Retrieves the "version"  from panda series.
        # In this case I can' t call sstv.item() on an empty series
        # Should "try/except" be used instead this? Probably yes.
        # If is the best solution, I don't know.
        sstv = ssts['Version']
        if sstv.empty:
            storedversion=0
        else:
            storedversion = sstv.item()

        # Verify if version found is newer than stored on control file
        if version > storedversion:

            # get_best_depot() returns a single hostname that can hold the file for backup
            dstdepot = get_best_depot()

            if ssts.empty:
                # data Serie is empty. Add a new entry (to be later wrote on local filesystem)

                # Stores copy the f file to the correct location on dstdepot
                if store(f,dstdepot):
                    # If successfuly stored, add to the stored files dataframe
                    sfdf=sfdf.append(pandas.DataFrame([[ f, version, dstdepot ]], columns=cfcolumns),ignore_index=True)
                else:
                    # Fails and exit.. Needs to correctly handle this
                    sys.exit(1)
            else:
                # File exists but in older version

                # Get actual depot for the file from "storedfiles"  dataFrame (sfdf)
                dstdepot = get_depot(f,sfdf)

                # Send the file to the depot server.
                if store(f,dstdepot):
                    dstidx = ssts.index[-1]
                    sfdf.loc[dstidx]=[ f,version, dstdepot ]
\$\endgroup\$
  • \$\begingroup\$ What is the dsts variable? You are testing against dsts.empty but you never set dsts. Is your code actually working? \$\endgroup\$ – holroy Feb 16 '16 at 0:46
  • \$\begingroup\$ @holroy I'd guess that's a typo and should be dstdepot, can you confirm that Lin? \$\endgroup\$ – SuperBiasedMan Feb 16 '16 at 12:12
  • \$\begingroup\$ It should be ssts instead dsts. ssts is the panda "sub" dataframe that contains all files such filename field from storedfiles matches with f on for loop from filelist.filename (filelist cames from a list of files candidates to the backup). \$\endgroup\$ – Lin Feb 16 '16 at 13:03
  • \$\begingroup\$ If all this variable names looks messy, is because they are messy. I'm looking for a way to write this better. Should I use snake_case variable naming instead those acronyms? \$\endgroup\$ – Lin Feb 16 '16 at 14:31
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathieu Guindon Feb 16 '16 at 19:49
2
\$\begingroup\$

In general, iterating through the rows of a series or dataframe is slow, and is not the recommended process. Instead, you should do one of two things:

  1. use map or apply (map for series, generally apply for data frame; see answers to this question for more details)
  2. select/broadcast

In this case, you have the opportunity to do both. You should use apply to get the version information for your filelist dataframe:

def function_that_does_your_xml_stuff(row):
    """Get version information for a row in the filelist dataframe from XML.

    Your code above sets the filename as f, but then doesn't seem to use it
    which I find confusing.  Perhaps it was a typo, and `a` should be `f`?
    Of course, a vanilla string object doesn't have a get_xml() method.

    """
    filename = row['filename']
    xmlfile = a.get_xml(filename)
    xmldata = a.load(xmlfile)
    version = int(xmldata.get_data_created().strftime("%s"))
    return version

filelist['Version'] = filelist.apply(function_that_does_your_xml_stuff, axis=1)

(the axis=1 argument here tells apply to work on rows instead of columns). This adds a new column to your filelist dataframe which contains the version information as obtained from your XML data associated with each file name.

Then, to compare with your already stored versions, you will use a selector based on your versioning criterion. Actually, in this case, you're going to want to perform a join on the two data frames first, and then use the selector:

filelist = filelist.join(sfdf, how=left, on='Filename', rsuffix='_stored')
need_to_update = filelist[filelist['Version'] > filelist['Version_stored']]
need_to_update.apply(actually_update, axis=1)

The join here accomplishes the task of connecting the files that have already been backed up with those in filelist; the second line selects out only those files whose current version is greater than the one that was stored previously, and the third line actually performs the update (again, by applying a function).

\$\endgroup\$
  • \$\begingroup\$ filelist pandas.DataFrame doesn't contains 'Version' row. It does have a Created row that outputs something like 12-Feb-15 10:10:59. I try to not rely on filelist output, since it cames from another dev team (many issues involved ;-). With the filelist (and therefore with the file names). I fetch the other attributes from XML data related to that filename. Of course, I can rely on filelist structure and kick off the XML stuff. would make things simpler, but I still need to convert the "Created" field to something like 'version', already handeld on my XML lib. \$\endgroup\$ – Lin Feb 16 '16 at 17:17
  • \$\begingroup\$ I was going to respond to your comment with another comment, but I'll just edit my answer, instead. \$\endgroup\$ – tachycline Feb 16 '16 at 17:27
  • \$\begingroup\$ I have enhanced the question to give a better view of the data types. \$\endgroup\$ – Lin Feb 16 '16 at 17:40
  • \$\begingroup\$ Nice idea. And yes. I haven't used f. Fixed that in original post. (data should be read as f). \$\endgroup\$ – Lin Feb 16 '16 at 17:42
  • \$\begingroup\$ I would say that this apply method is amazing. Nice hint. \$\endgroup\$ – Lin Feb 16 '16 at 17:51

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.