Installing "mods" with Python

Question

A little disclaimer: you're going to have a field day with this. It's horrible. Looking at it makes me want to vomit, and don't ask me to explain it, because I've long forgotten what I was thinking when I wrote it. That being said, I can't really find a way to shorten it of make it make sense. Perhaps someone could help me out?

A bit of a background:

This program is meant to install mods for a game called Supreme Commander 2. It is meant to install ANY mod. The following is true of all the mods:

• The only archive type is the .scd. SCD's have the same encoding as .zip's, but with a 0% compression rate.

• The way to install the mod is simple: There will always be at least one scd inside the first, and there may be more scd's inside that one. Any scd with folders inside it should be moved to

  C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata

  There may be more than one scd to be moved per mod.

And that's it. This is something of a "programming challenge," but I am really looking for improvements upon the following code:

import os, zipfile
def installfromdownload(filename):
    global scdlist
    targetfolder = r"C:\Program Files (x86)\Mod Manager\Mods\f_" + filename[:-4]
    lentf = len(targetfolder)
    unzip(r"C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd", targetfolder)
    if checkdirs(targetfolder) == False:
        os.system("copy C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd" " C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata")
    else:
        newfolderonelist = []
        getscds(targetfolder)
        for file in scdlist:
            newfolderone = targetfolder + "\f_" + file[:-4]
            filepath = targetfolder + "\\" + file
            unzip(filepath, newfolderone)
            newfolderonelist.append(newfolderone)
        for newfolders in newfolderonelist:
            if os.checkdirs(newfolders) == False:
                newfolders = newfolders[lentf + 1:]
                for scds in scdlist:
                    if newfolder in scds == True:
                        scdpath = targetfolder + "\\" + scds
                        os.system("copy " + scdpath + " C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata")

Note that there is no error handling here, that is done when I call the function. The function unzip() is here:

def unzip(file, target):
    modscd = zipfile.ZipFile(file)
    makepath(target)
    modscd.extractall(target)

The function checkdirs() is here (note this is pretty bad, it only checks for periods in the file name, so please submit suggestions):

def checkdirs(target):
    isfile = 0
    targetlist = os.listdir(target)
    for files in targetlist:
        for letters in files:
            if letters == ".":
                isfile = isfile + 1
    if isfile == len(os.listdir(target)):
        return False
    else:
        return True

The getscds() function just checks a filename for ".scd".

Answer 1

Your main function is hard to follow, but it seems to repeat its logic with subfolders. Generally something like that should be implemented as a recursive function. In pseudo-code, the basic structure of the recursive function would be:

def copy_scds(scd, target_folder):
    unzip scd to target_folder

    if target_folder has scds:
        for each inner_scd:
            subfolder = new folder under target_folder
            copy_scds(inner_scd, subfolder)
    else:
        copy scd to gamedata

As you can see, the unzip and copy parts are only written once, as opposed to repeated in nested for loops. The additional scds and subfolders are simply passed in as new arguments for scd and target_folder.

The main function then only needs to determine the initial "root" scd and target_folder and begin the process:

def install(filename):
    scd = r"C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd"
    target_folder = r"C:\Program Files (x86)\Mod Manager\Mods\f_" + filename[:-4]

    copy_scds(scd, target_folder)

My only other suggestion other than what was given in the other answers is to store the paths to make them more easily configurable and eliminate repetition in the code. For example:

mods_folder = r"C:\Program Files (x86)\Mod Manager\Mods"
zip_prefix = "z_"
output_prefix = "f_"
gamedata_folder = r"C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata"

In fact, it may be possible to completely remove the need to configure the game folder. Steam knows how to launch any game based only on a unique AppID, so theoretically there must be some way to determine the path automatically.

Answer 2

Firstly, your installfromdownload function should be install_from_download (and have a docstring to go with it explaining what it does).

This line:

targetfolder = r"C:\Program Files (x86)\Mod Manager\Mods\f_" + filename[:-4]

should probably check and make sure that the file actually ends with .zip (or whatever format you're actually expecting it to come in):

if filename.endswith('.zip'):
    target_folder = ...

Something doesn't seem quite right with:

unzip(r"C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd", targetfolder)

You're stripping the last 4 characters off filename before, and here you're appending '.scd'. I assumed you were stripping .scd off above, but then realized you're unzipping it, so it's actually a .zip (or something like that). Either way, it's confusing and something like:

target_folder = r"..." + filename.split('.zip')[0] 

would make it much more clear what you're actually doing (commenting would help a bit too).

Don't explicitly check against False:

if checkdirs(targetfolder) == False:

This should be:

if not checkdirs(targetfolder):

Don't join paths together with "\\" (which is OS specific anyway):

 filepath = targetfolder + "\\" + file

Use os.path.join instead:

 filepath = os.path.join(target_folder, file)

Some of the variable names here make your code really, really hard to follow (newfolderonelist, newfolderone).

This line should make your script crash:

if os.checkdirs(newfolders) == False:

As you're trying to call your checkdirs function (os.checkdirs doesn't exist).

Let's look at the checkdirs function:

def checkdirs(target):
    isfile = 0
    targetlist = os.listdir(target)
    for files in targetlist:
        for letters in files:
            if letters == ".":
                isfile = isfile + 1
if isfile == len(os.listdir(target)):
    return False
else:
    return True

Again following Python naming convention, this should be check_dirs (although the name needs a lot of work - what's it checking them for?). Further, a lot of the work here can replaced with os.path.isfile. From reading it, it looks like you're checking that everything in a given directory is not a file, so I'm going to call it not_all_files:

def not_all_files(directory):
    '''Checks the given directory, returning False if it contains only files,
    and True otherwise. If the argument given is not a directory, 
    raises IOError.
    '''

    if not os.path.isdir(directory)
        # Oops, passed in something that wasn't a directory
        # Handle this error
        raise IOError('....')
    return not all((os.path.isfile(f) for f in os.listdir(directory)))

Given how you're using it above, I'd rather swap around what it returns: have it return True if everything in the directory is a file, False otherwise. This would then be:

def all_files(directory):
    '''Checks the given directory, returning True if it contains only files,
    and False otherwise. If the argument given is not a directory, 
    raises IOError.
    '''

    if not os.path.isdir(directory)
        # Oops, passed in something that wasn't a directory
        # Handle this error
        raise IOError('....')
    return all((os.path.isfile(f) for f in os.listdir(directory)))

Your code in install_from_download would then become:

 if all_files(target_folder):

which I think is much easier to read.

Answer 3

In addition to Yuushi's answer, I notice is that you're using os.system for copying files. This is insecure and unnecessary. If all you need to do is copy a file, use shutil.copy.

Translated, a bit of your code might look like this:

# assuming shutil is imported
shutil.copy(r"C:\Program Files (x86)\Mod Manager\Mods\z_" + filename + ".scd",
            r"C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata")

Of course, you'd then want to change that code to use os.path.join, as suggested in Yuushi's answer.