Python RESTful image handler

Question

This is a view as part of a RESTful image handler module, written for Flask, I am sure there is a better/more pythonic way of doing parts of this, maybe using List Comprehensions or Generator Functions. But I am a bit of a Python newbie and cannot spot it. Can any experienced pythonistas take a look?

@__mod__.route('/', methods=['GET'])
@__mod__.route('/<filename>', methods=['GET'])
def get(filename=None):
    """ Get a list of all Images or a single Image """

    couchdb.init_app(current_app)
    image = Image()

    if filename is None:
        results = image.view('images/all')
        filenames = []
        for result in results.all():

            if result._attachments:
                for attachment in result._attachments.keys():
                    filenames.append(url_for('.get', filename=attachment))
        response = make_response(simplejson.dumps(filenames), 200)
        response.headers['Content-Type'] = 'application/json'
        return response
    else:
        results = image.view('images/all', key=filename)

        if results.first() is not None: 
            try:
                image_file = image.get_db().fetch_attachment(results.first(), filename)
                image_doc = results.first()._attachments[filename]
                response = make_response(image_file, 200)
                response.headers['Content-Type'] = image_doc['content_type']
                return response
            except ResourceNotFound:
                abort(404)

        else:
            abort(404)

I've managed this on my own, using a list comprehension instead of the ugly for-if structures:

if filename is None:
    results = image.view('images/all')          
    filenames = [url_for('.get', filename=attachment) 
                 for result in results 
                 if result._attachments
                 for attachment in result._attachments]
    response = make_response(simplejson.dumps(filenames), 200)
    response.headers['Content-Type'] = 'application/json'
    return response
else:
    results = image.view('images/all', key=filename)

    if results.first(): 
        try:
            image_file = image.get_db().fetch_attachment(results.first(), filename)
            image_doc = results.first()._attachments[filename]
            response = make_response(image_file, 200)
            response.headers['Content-Type'] = image_doc['content_type']
            return response
        except ResourceNotFound:
            abort(404)
    else:
        abort(404)

Answer 1

@__mod__.route('/', methods=['GET'])
@__mod__.route('/<filename>', methods=['GET'])

Seeing as you end up handling these two routes in completely different ways, why don't you just implement two functions?

def get(filename=None):
    """ Get a list of all Images or a single Image """

    couchdb.init_app(current_app)
    image = Image()

I don't know what these class does, but I suspect its not really an Image given the methods called on it. It seems more like an image library or something.

    if filename is None:
        results = image.view('images/all')
        filenames = []
        for result in results.all():

            if result._attachments:

Attributes starting with underscores are usually intended to be private. You usually shouldn't be accessing them.

                for attachment in result._attachments.keys():

If you are checking for an empty _attachments above (as opposed to None), you don't need to. The loop will execute zero times for an empty dictionary. Also, you don't need keys() because the default iteration for a dictionary is keys

                    filenames.append(url_for('.get', filename=attachment))

You did post an edit with a comprehensionified version.

filenames = [url_for('.get', filename=attachment) 
             for result in results 
             if result._attachments
             for attachment in result._attachments]

The same issues exist here as with the explicit for loop. Its also a bit long and complex I might do the following

attachments = [result._attachments for result in results if result._attachments]
attachments = [attachment for attachment for attachments in attachments]
attachments = [url_for('.get', filename = attachment) for attachment in attachments]

Better? maybe. maybe not.

        response = make_response(simplejson.dumps(filenames), 200)
        response.headers['Content-Type'] = 'application/json'
        return response
    else:
        results = image.view('images/all', key=filename)

        if results.first() is not None: 
            try:
                image_file = image.get_db().fetch_attachment(results.first(), filename)
                image_doc = results.first()._attachments[filename]
                response = make_response(image_file, 200)
                response.headers['Content-Type'] = image_doc['content_type']
                return response

I'd consider a utility function that calls make_response and sets a content_type.

            except ResourceNotFound:
                abort(404)

It is recommended that you put as little as possible in your try blocks. You only want to catch exceptions from where it is expected to be thrown. I'm not sure where you it is coming from in your code.

        else:
            abort(404)