5
\$\begingroup\$

Please review this:

from os import path, remove

try:
    video = Video.objects.get(id=original_video_id)
except ObjectDoesNotExist:
    return False

convert_command = ['ffmpeg', '-i', input_file, '-acodec', 
       'libmp3lame', '-y', '-ac', '2', '-ar', 
       '44100', '-aq', '5', '-qscale', '10', 
       '%s.flv' % output_file]                     

convert_system_call = subprocess.Popen(
    convert_command,
    stderr=subprocess.STDOUT,
    stdout=subprocess.PIPE
)

logger.debug(convert_system_call.stdout.read())

try:

    f = open('%s.flv' % output_file, 'r')
    filecontent = ContentFile(f.read())
    video.converted_video.save('%s.flv' % output_file, 
                     filecontent, 
                     save=True)
    f.close()

    remove('%s.flv' % output_file)

    video.save()

    return True
except:
    return False
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Clean, easy to understand code. However:

Never hide errors with a bare except. Change

try:
    ...
except:
    return False

into

try:
    ...
except (IOError, AnyOther, ExceptionThat, YouExpect):
    logging.exception("File conversion failed")

Also, unless you need to support Python 2.4 or earlier, you want to use the files context manager support:

with open('%s.flv' % output_file, 'r') as f:
    filecontent = ContentFile(f.read())
    video.converted_video.save('%s.flv' % output_file, 
                               filecontent, 
                               save=True)
remove('%s.flv' % output_file)

That way the file will be closed immediately after exiting the with-block, even if there is an error. For Python 2.5 you would have to from __future__ import with_statement as well.

You might also want to look at using a temporary file from tempfile for the output file.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks a lot for the review! I have updated the snippet to be pastebin.com/9fAepHaL But I am confused as to how to use tempfile. The file is already created by the "convert_system_call" before I use it's name in the try/catch block. Hmm,, maybe I use tempfile.NamedTemporaryFile()? \$\endgroup\$
    – Chantz
    Feb 1, 2011 at 15:15
  • \$\begingroup\$ @Chantz: Yeah, exactly. If not you may end up with loads of temporary files in a current directory, where users won't find them and they ever get deleted. \$\endgroup\$ Feb 1, 2011 at 15:43
  • 1
    \$\begingroup\$ Lennart following your & sepp2k's suggestion I modified my code to be like pastebin.com/6NpsbQhz Thanks. \$\endgroup\$
    – Chantz
    Feb 2, 2011 at 16:43
3
\$\begingroup\$

In addition to the points that Lennart made, there's just one minor thing I'd like to add:

You repeat the expression '%s.flv' % output_file four times in your code. You might want to store this in a variable like filename = '%s.flv' % output_file. This way there's less repetition and if you ever want to change the target file format, all you'll have to change are the filename and convert_command variables.

\$\endgroup\$
2
  • \$\begingroup\$ Makes sense. Will make the change. Thanks! \$\endgroup\$
    – Chantz
    Feb 1, 2011 at 18:18
  • \$\begingroup\$ sepp2k following your & Lennart's suggestion I modified my code to be like pastebin.com/6NpsbQhz Thanks. \$\endgroup\$
    – Chantz
    Feb 2, 2011 at 16:43

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.