5
\$\begingroup\$

I'm new to Python and wrote this code for collecting all files in an m3u (playlist) file and copying them into a directory.

import shutil
import os
import sys

args = sys.argv

#check for m3u file
if len(args) > 1 :   
    m3ufile = args[1]
else :
    print "m3u file must be set"
    sys.exit()

#check for destination arg
try :
    dest = args[2]
except IndexError :
    dest = '.'

#check for destination exists
if os.path.exists(dest) != True and dest != '.' :
    os.makedirs(dest)

files = []     
with open(m3ufile) as f:
    for t in f :
        if t[0] != '#' and t not in ['\n','\r\n'] :
            t = t.strip()
            if t :
                files.append(t)

x = 0
fp = len(files)

while x < fp :
    print(chr(27) + "[2J")
    print str(x + 1) + ' of ' + str(fp) + ' collected!!'
    if os.path.exists(files[x]) : 
        shutil.copy( files[x] , dest)
    x = x + 1

print "all files collected in " + dest + " directory.enjoy!"
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I recommend that you add more to your post. What are you looking for in a review? And, possibly, what is your code doing to accomplish the task you describe? \$\endgroup\$ – SirPython Oct 16 '15 at 23:25
  • \$\begingroup\$ Are you using Python 2.7 or 3.x? It's relevant to one of the suggestions given in an answer. \$\endgroup\$ – SuperBiasedMan Oct 19 '15 at 11:30
5
\$\begingroup\$

Command-line handling

Fatal errors should be reported on sys.stderr and cause a non-zero exit status.

Don't check for os.path.exists(dest), because:

  • Existence doesn't actually guarantee that it is a directory rather than a regular file.
  • os.makedirs(dest) will perform that check anyway.
  • Even if you os.path.exists() finds a directory, there is a slim chance that it might get deleted in a race condition.

If you want to catch IndexError instead of checking for the length of sys.argv, then I suggest that you do so consistently for both arguments:

from __future__ import print_function

try:
    m3ufile = sys.argv.pop(1)
except IndexError:
    print("No m3u file given", file=sys.stderr)
    sys.exit(1)

try:
    dest = sys.argv.pop(1)
except IndexError:
    dest = '.'
else:
    os.makedirs(dest)

File input

t is a funny variable name. How about line instead?

t not in ['\n','\r\n'] is superfluous, since if t performs the same check.

files = []
with open(m3ufile) as f:
    for line in f:
        line = line.strip()
        if line and line[0] != '#':
            files.append(line)

(This isn't exactly equivalent to your original code, in that it also treats # as a comment even if it is preceded by whitespace. I consider that to be an improvement.)

Copying

The Esc[2J VT100 escape code deserves a comment. Also consider using curses for clarity and generality.

You shouldn't declare that something is done until it's done, especially if there's a chance that you might not do it at all. In fact, I would expect that a missing file would be considered an error.

I suggest using itertools.count() to track the progress.

from itertools import count

progress, goal = count(1), len(files)
skipped = []
for path in files:
    # Esc [2J is the VT100 sequence to erase the screen and move the cursor to
    # the beginning of the line
    if os.path.exists(path):
        shutil.copy(path, dest)
        print("\x1b[2J{0} of {1} collected!!".format(next(progress), goal))
    else:
        skipped.append(path)

if skipped:
    print("Missing files:", file=sys.stderr)
    for path in skipped:
        print(path, file=sys.stderr)
    sys.exit(2)
else:
    print("All files collected in {0} directory.  Enjoy!".format(dest))
\$\endgroup\$
  • \$\begingroup\$ thanks for your reply. I have another question. if m3u file not found I will get an error. how can I handle it ? \$\endgroup\$ – Saeed M. Oct 17 '15 at 19:40
  • \$\begingroup\$ "makedirs will perform that check anyway" the doc says the opposite, it says an error will be raised if you try and make an existing folder. Though on Python 3 it seems that they added an optional parameter where you can tell it to ignore such errors. \$\endgroup\$ – SuperBiasedMan Oct 19 '15 at 10:20
3
\$\begingroup\$

Don't use os.path.exists(dest) != True, just use if not os.path.exists(dest). In general, test booleans simply like this instead of using equivalence for them.

x = x + 1 can be rewritten much easier as x += 1.

You should also use str.startswith instead of indexing the zero value, as it's more readable.

t.startswith('#')

And you should use string formatting. It makes it easier to insert values into strings, like this:

str(x + 1) + ' of ' + str(fp) + ' collected!!'

can be turned into this:

'{} of {} collected!!'.format(x + 1, fp)

It automatically converts the parameters to strings, and then places them where the {} are in your string literal.

\$\endgroup\$

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.