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\$
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 '11 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\$ – Lennart Regebro Feb 1 '11 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 '11 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 '11 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 '11 at 16:43

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.