2

I have a class with a Django ImageField and I have been struggling to decide between two alternatives for storing that field's upload_to function. The first approach is pretty straightforward. The function is defined on the module level (c.f. https://stackoverflow.com/a/1190866/790075, https://stackoverflow.com/a/3091864/790075):

def get_car_photo_file_path(instance, filename):
    ext = filename.split('.')[-1]
    filename = "%s.%s" % (uuid.uuid4(), ext) # chance of collision <1e-50
    return os.path.join('uploads/cars/photos', filename)

class CarPhoto(models.Model):
    photo = models.ImageField(upload_to=get_car_photo_file_path)

This is simple and easy to understand, but pollutes the module scope by adding a function that is really only pertinent to the CarPhoto class.

In the second approach, I use the callable-class pattern to associate the function more closely with the CarPhoto class. This moves the upload_to function out of module scope but feels unnecessarily complicated.

class CarPhoto(models.Model):
    class getCarPhotoFilePath():
        # Either use this pattern or associate function with module instead of this class
        def __call__(self, instance, filename):
            ext = filename.split('.')[-1]
            filename = "%s.%s" % (uuid.uuid4(), ext) # chance of collision <1e-50
            return os.path.join('uploads/cars/photos', filename)

    photo = models.ImageField(upload_to=getCarPhotoFilePath())

I have seen suggestions for using the @staticmethod and @classmethod decorators (c.f. https://stackoverflow.com/a/9264153/790075), but I find that when I do this the function never executes and the filename ends up looking like: /path/to/file/<classmethod object>, with the method object embedded in the file path, which is certainly not intended!

Which of these is the preferred pattern? Is there a better way?

Community
  • 1
  • 1
turtlemonvh
  • 9,149
  • 6
  • 47
  • 53
  • BTW you can avoid defining a function / method / class altogether and turn your `get_car_photo_file_path`into an inline lambda right inside `ImageField` invocation. But this probably be totally unreadable. – 9000 Oct 02 '12 at 16:34

3 Answers3

1

I would recommend that you:

import this

To me, this falls under the Zen of Python's section stating:

Simple is better than complex.
Complex is better than complicated.

I think your simple solution is better. But, your complex doesn't feel overly complicated. I think you will probably be ok either way. Just my two cents.

David S
  • 12,967
  • 12
  • 55
  • 93
  • I was hoping for something more concrete, but I can't say I don't agree with you! – turtlemonvh Oct 02 '12 at 16:17
  • Sorry. I tried to be pretty concrete and say that I think the simple solution (the first one) is better. And the reason why is that I think it is more closely adheres to the "Zen of Python". But, the solution with the Callable is not terrible. I just prefer the first one a bit. – David S Oct 02 '12 at 16:21
0

There's a naming convention to prevent name pollution.

  • use _get_car_photo_file_path to mark your function as internal (though not hidden);
  • use __get_car_photo_file_path to prevent access to it from outside your class.

You can add a classmethod or staticmethod like this to your CarPhoto class, which is simpler than adding a callable class (the latter reminds me of Java's way to define an anonymous class for the sake of one method).

The name will cleanly show that _get_car_photo_file_path is an implementation detail and not a part of the interface, thus preventing pollution of class's namespace. Being CarPhoto's method, the function will not pollute module's namespace.

9000
  • 39,899
  • 9
  • 66
  • 104
0

Currently in the code I'm working with we have the variation of the simplest one. The only difference is that since the function is intended for internal use, it's marked so with _ prefix.

def _get_car_photo_file_path(instance, filename):
    [...]

class CarPhoto(models.Model):
    photo = models.ImageField(upload_to=_get_car_photo_file_path)

However, I do believe this would be more Pythonic (or rather more OOP):

class CarPhoto(models.Model):

    @staticmethod
    def _get_file_path(instance, filename):
        [...]

    photo = models.ImageField(upload_to=_get_file_path)
vartec
  • 131,205
  • 36
  • 218
  • 244
  • The second suggestion was what I tried first, but this fails because you don't have access to `CarPhoto` (the class) or `self` here. See comments http://stackoverflow.com/a/3091717/790075 – turtlemonvh Oct 03 '12 at 17:31
  • @turtlemonvh: static method doesn't need `self`. – vartec Oct 03 '12 at 18:11
  • @turtlemonvh: I fail to see how you use `self` of class inside the function that calculates the file path. The `instance` parameter is never used in the function from your post. – 9000 Oct 04 '12 at 18:55
  • @vartec True, but it does need the class, which is also not available – turtlemonvh Oct 04 '12 at 20:40
  • @9000 That's correct: I don't use `self` in the function. The name of the file is generated as a uuid and is not derived based on any property of the file itself. My point is not that I need `self` to do the work of the function, but that calling either a `staticmethod` or a `classmethod` requires access to either `self` or `self.__class`, both of which are unavailable. – turtlemonvh Oct 04 '12 at 20:43
  • @turtlemonvh: staticmethod does _not_ require access to self or class. Please see http://pastebin.com/VCy8q3A8 and run it in Python REPL. If in doubt, try to experiment in REPL a bit before giving up or making a statement. I won't dare make the statement I'm making without checking it first with a piece of code. – 9000 Oct 04 '12 at 20:53