0
\$\begingroup\$

I'm quite a newbie with Python, but I wanted to try my own little project, so I made a small script that puts all the desktop icons/folders into a newly created folder (called "Desktop1") if there are more than 25 items on the desktop.

import os
import shutil

#My user on windows is Shadow so you're not like 'huh??'

os.chdir("C:\Users\Shadow\Desktop")
desktop = os.listdir("C:\Users\Shadow\Desktop")
desktopFiles = []

#Folder that the desktop files/icons will move to once created
newFolder = "C:\Users\Shadow\Desktop\Desktop1" 

#Function that does the moving
def moveFilesToFolder(files, directory, folder): 
    for files in directory:
        shutil.move(files, folder)

#Place the files into a list so I can determine how many there are
for x in desktop:                       
    desktopFiles.append(x)

if len(desktopFiles) > 25:    #If there's more than 25 files/icons on the desktop

   #If you already have an EMPTY folder named Desktop1 on your Desktop 
    if os.path.exists(newFolder) and os.listdir(newFolder) == []: 
        moveFilesToFolder(file, desktop, newFolder)  
        print "Desktop1 already exists and is empty, so the files have been placed there"


    else:                 #Otherwise, make Desktop1 and move the icons/files into it
        os.mkdir(newFolder)
        moveFilesToFolder(file, desktop, newFolder)

What can I do to make it better? It works well and gets the job done, but is there any improvement I can make to it now (performance, syntax, etc.) that I can use for any later programs I may want to create? I feel like I'm not using object oriented programming at all.

\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

You hard-coded "C:\Users\Shadow\Desktop" three times. Once would have been too many, as it turns a general-purpose script into a throwaway script. Your script could take a command-line parameter or infer the desktop directory for the current user.

If you must hard-code a path as a string literal, use raw strings to prevent Python from interpreting special backslash escape sequences such as \n for Newline.


Let's focus on this function:

#Function that does the moving
def moveFilesToFolder(files, directory, folder): 
    for files in directory:
        shutil.move(files, folder)
  1. What is files? It's a parameter that you accept but never use. Rather, you reuse the same variable name as a dummy variable for iteration.
  2. Using an iteration variable with a plural name is awkward.
  3. What do the parameters directory and folder mean? How about renaming directoryfiles and folderdest_dir for clarity?

I was curious how you actually call that function, given the weird parameter design.

moveFilesToFolder(file, desktop, newFolder)

At first glance, it appears that the script should fail, since file is never defined as a variable in your script. However, it turns out that file is the name of a built-in constructor function. So, you're passing some arbitrary function as a parameter to a function that ends up doing nothing with it. I find that rather funny.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for this! I used your suggestions and made a lot of changes to the code and its much clearer/more general-purpose oriented \$\endgroup\$
    – Shadow
    Sep 16, 2014 at 18:04
1
\$\begingroup\$

Here is what I see:

  • Put moveFilesToFolder to the top of the file ( but below the imports ).
  • Check the length of desktop, and if it is less than 25, quit. This would be done before appending to desktopFiles
  • Does desktopFiles even need to exist?
  • Both branches of the if/else statement move the files, so they can be brought to after that statement

Some added features I can think of:

  • Make the target directory a command line argument so I can say where the files get moved to.
  • Add an option for if the program should create the directory if it doesn't .exist
  • and an option to be safe and not overwrite existing files.
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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