Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My original code is here. I made some changes to it based on your suggestions, but then I lost my flash drive and had to redo it. I have a suspicion that my second version was better than this one, so could you all help me out one more time?

import os, shutil, zipfile

def subtractname(path):
    newpath = str()
    path = path.split("\\")
    path.pop()
    for x in range(len(path) - 1):
        newpath += (path[x] + "\\")
    newpath += path[len(path) - 1]
    return newpath

def pathtoname(path):
    path = path.split("\\")
    newpath = path[len(path) - 1]
    newpath = newpath[:-4]
    return(newpath)

def makepath(path):
    try:
        os.makedirs(path)
    except FileExistsError:
        shutil.rmtree(path)
        os.makedirs(path)

def getscds(targetfolder):
    global scdlist
    filelist = os.listdir(targetfolder)
    scdlist = []
    for file in filelist:
        if '.scd' in file:
            scdlist.append(os.path.join(targetfolder, file))
    return scdlist

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

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

def install(folderlist):
    newpathlist = []
    for folder in folderlist:
        pathlist = getscds(folder)
        for path in pathlist:
           newpath = os.path.join(subtractname(path), ("f_" + pathtoname(path)))
           newpathlist.append(newpath)
           unzip(path, newpath)
           if checkdirs(newpath):
            shutil.copy(path, r"C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata")
            newpathlist.remove(newpath)
    return newpathlist

a = input()
b = [a]
install(install(install(b)))

A note: this code SHOULD actually work if you run it, but I haven't incorporated error handling yet.

I know that the last three lines can be condensed into one, but aside from that is there anything that could be improved here? I'm not really looking for naming conventions or comments, but I'm fine with it if you want to throw them in along with some more practical suggestions.

EDIT: I just tried running this on a friend's computer, and it may not work after all. Ideas?

share|improve this question
    
Hello! Would you mind mentioning which version of Python you're running? –  Jeff Gohlke Apr 14 at 16:19
    
@Jeff Python 3.3 –  KnightOfNi Apr 14 at 16:34

1 Answer 1

I'll leave an in-depth analysis to the true Python experts, but here are just some quick notes:

Variable and function names: As far as I'm aware, variable and function names should have words/elements separated by underscores (e.g., check_dirs, is_file)

main() function: Instead of having your functionality on the first/default level of indentation, it's good form to wrap it in a def main() and call it via:

if __name__ == '__main__':
    main()

Return values: You can simply return a bool directly rather than having an if/else construction. e.g.,

return isfile != len(os.listdir(target))

vs.

if isfile == len(os.listdir(target)):
    return False
else:
    return True

Use of global: In my experience, any time I have to use global some_var, it's indicative of bad design. You should either be using local variables, function arguments, or the constant should be well-defined and not hidden by any local fields.

share|improve this answer
    
Yeah, the global thing was left over from version 1. I'll take it out as it doesn't seem to serve a purpose anymore. –  KnightOfNi Apr 14 at 16:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.