Installing "mods" with Python

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

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.

StackExchange Code Review Q#41531, answer score: 13