9
\$\begingroup\$

Task:

Given an image object and an effect identifier (string), apply the effect identified by the string on the image. The library of effects can grow as the program grows.

Code 1:

def apply_fx(img, fx):
    if fx == 'sepia':
        return sepia(img)
    elif fx == 'b&w':
        return contrast(grayscale(img), 2)
    elif fx == 'faded':
        return faded(img)
    elif fx == 'hdr':
        return hdr(img)

Code 2:

effects = {
    'sepia': sepia,
    'b&w': lambda x: contrast(grayscale(x), 2),
    'faded': faded,
    'hdr': hdr
}

def apply_fx(img, fx):
    return effects[fx](img)

Opinion: I personally lean toward the second approach.

Note: the concern here generalizes to: dictionary of lambda functions vs if-elif in such scenarios.

Extra code: as suggested by @PeilonRayz.

sepia_filter = np.array([[.393, .769, .189],
                         [.349, .686, .168],
                         [.272, .534, .131]])


def sepia(img):
    sepia_img = np.dot(img, sepia_filter.T)
    return sepia_img / np.max(sepia_img)
\$\endgroup\$
  • 1
    \$\begingroup\$ Please can you add more code, how are sepia etc. defined? Are they all on one class? Can they all be defined on one class? \$\endgroup\$ – Peilonrayz Jan 5 '18 at 12:02
  • \$\begingroup\$ Module-level functions. \$\endgroup\$ – Adel Redjimi Jan 5 '18 at 12:59
  • \$\begingroup\$ Please provide the code. \$\endgroup\$ – Peilonrayz Jan 5 '18 at 13:06
  • \$\begingroup\$ @AdelRedjimi Is the apply_fx also provided in the same module or is it defined in code importing the module? \$\endgroup\$ – Mathias Ettinger Jan 5 '18 at 13:06
  • \$\begingroup\$ For now, same module. \$\endgroup\$ – Adel Redjimi Jan 5 '18 at 13:17
5
\$\begingroup\$

IMO your second solution (using a dict to store callables) is exactly right.

The first solution, with a series of if/else statements is not only inefficient, it also encourages and facilitates writing unrelated code inside the apply_fx function itself. To wit:

elif fx == 'b&w':
    return contrast(grayscale(img), 2)

Once you have put something other than a simple return func(img) in the code, a future maintainer will be tempted to "just write this inline here" rather than creating a lambda or a named function to encapsulate their code:

elif fx = 'grayscale':
    from thirdpartymodule import grayscale, formatconverter

    gsh = grayscale.Handle()
    img3p = formatconverter(img)
    result = grayscale.grayscale(gsh, img3p)
    return formatconverter(img3p, format=img)

Your second alternative, on the other hand, has a much more restricted, and thereby much cleaner interface: all operations must be encapsulated as callables.

Now, they could be functions, or they could be lambdas. They could be functools.partials or dynamically loaded or web api calls to a satellite orbiting planet Nebulon. But from the maintenance point of view, "create a callable, plug it in here with a name" is as good as you're likely to get. You could even do it dynamically, in response to a configuration file or plugin module:

def img2grayscale(img):
    from thirdpartymodule import grayscale, formatconverter

    gsh = grayscale.Handle()
    img3p = formatconverter(img)
    result = grayscale.grayscale(gsh, img3p)
    return formatconverter(img3p, format=img)

effects['grayscale'] = img2grayscale
\$\endgroup\$
3
\$\begingroup\$

It appears to be premature optimisation problem. However; the 2nd approach will definitely be easier to maintain. If you wish, even the apply_fx can be implemented as a lambda call.

You can dig deeper into the effects of if-else nest vs. dict lookup here.

\$\endgroup\$
3
\$\begingroup\$

I second @Austin Hastings' answer but I'd like to propose a third approach. Depending on your needs this can be limited at will.

The fact is that your effects dictionary is almost "just a namespace", but namespaces have already an implementation in Python: they are modules. So defining all the function in a specific module should be enough to "gather them under a namespace". It gives you the ability to retrieve them by their name through the module or using a string thanks to getattr.

So the simplest apply_fx implementation could be either

import image_manipulation_library

def apply_fx(img, fx):
    effect = getattr(image_manipulation_library, fx)
    return effect(img)

if defined outside of said module; or

import sys

def apply_fx(img, fx):
    effect = getattr(sys.modules[__name__], fx)
    return effect(img)

if defined inside.

The obvious advantage of this approach is that it forces you to define, name and optionally document a function for the black and white effect:

def black_and_white(img):
    return contrast(grayscale(img), 2)

Which is good for the users of your code as they can introspect it better. However this will limit the names to value Python identifiers.

You can also still plug third party code by just importing them, and you can limit the results of the getattr call to ensure it fits your needs (check it is a callable, inspect it's arguments list to verify it only has a single parameter...)

\$\endgroup\$
3
\$\begingroup\$

In addition to the recommendations suggested by @Austin, you could inherit from dict to create an object to register and recall your functions. For example:

class Effects(dict):
    def register(self, name):
        def decorator(f):
            self[name] = f
            return f
        return decorator

Effects = Effects()


@Effects.register('sepia')
def sepia(img):
    sepia_img = np.dot(img, sepia_filter.T)
    return sepia_img / np.max(sepia_img)

@Effects.register('b&w')
def b_w(img):
    return contrast(grayscale(img), 2)

# and so on...

Effects['sepia'](img)

Now you don't have to register each effect in your dictionary after the function declaration and this should clean things up a little. Additionally, if you import this object, you can add functions anywhere, easily see what effects are registered, and you can add logic to the Effects class itself if you want more complex behavior (i.e. address the case where an invalid string is entered).

\$\endgroup\$
2
\$\begingroup\$

From the looks of it, you want to have an Effects class, that can be used in the following ways:

Effects.sepia(img)
Effects['b&w'](img)

This is as you can define all your code as full on functions, and for you to not have \$O(n)\$ lookup. To do the above I'd use a metaclass that:

  • Implements a method, to allow you to look up by a different name than the function name.

    This is as b&w is an illegal function name in Python.

  • You want to group all the functions in the class by their name, or altered name.

  • You want to implement __getitem__ on a metaclass, so you can perform it on the class.

And so you can use:

class NamespaceMeta(type):
    def __new__(meta, classname, bases, class_dict):
        meta.lookup = lookup = {}
        for key, value in class_dict.items():
            if key[0] == '_':
                continue

            metadata = getattr(value, 'metadata', {})
            name = metadata.get('name', None)
            if name is None:
                name = key

            lookup[name] = value

        return type.__new__(meta, classname, bases, class_dict)

    def __getitem__(self, key):
        return self.lookup[key]


class Namespace(metaclass=NamespaceMeta):
    @staticmethod
    def metadata(name=None):
        def inner(fn):
            fn.metadata = {'name': name}
            return fn
        return inner

Using the above allows you to make a module that hides all the complexity, and then implement the classes with the complexity abstracted away.

class Effects(Namespace):
    def sepia(img):
        return f'sepia {img}'

    @Namespace.metadata(name='b&w')
    def b_w(img):
        return f'b&w {img}'


Effects.sepia('abc')    # sepia abc
Effects.b_w('abc')      # b&w abc
Effects['sepia']('abc') # sepia abc
Effects['b&w']('abc')   # b&w abc
\$\endgroup\$
  • \$\begingroup\$ In my undergrad years, I had courses assignments on Java. Having to write lots of class-like components is the reason why I didn't adapt Java (or any other OOP-focused language) as my primary weapon. Given the nature of the task discussed here, a functional approach seems to work better. \$\endgroup\$ – Adel Redjimi Jan 5 '18 at 13:47
  • 1
    \$\begingroup\$ That's the issue "it's more a namespace than a class". A class is a class of objects. We write classes to define new kinds of objects with properties and methods. A namespace is meant to group names together, avoid name collisions, and allow for names reuse in different contexts. Classes were not invented to act as namespaces. Python modules on the other side serve a similar purpose by grouping related functions and definitions and hanging their names to the module name. That's the exact issue with intrusive OOP, having to write classes to serve something else's purpose. \$\endgroup\$ – Adel Redjimi Jan 5 '18 at 15:38
  • 2
    \$\begingroup\$ @AdelRedjimi IMO neither FP or OOP are good. The above uses class, because it makes usage simple. \$\endgroup\$ – Peilonrayz Jan 5 '18 at 15:57
  • 1
    \$\begingroup\$ @Adel Redjimi Your arguments against this approach are unfair... cuz OOP? Personally I like this approach, because it is both versatile, and informative. \$\endgroup\$ – Ludisposed Jan 5 '18 at 16:23
  • 2
    \$\begingroup\$ This solution is wrong just because it stashes all the proposed methods in a single object. Your first illustrated example, using a dictionary of callables, is a better approach. It gives you the flexibility to add effects provided by a third party, using only a thin veneer of wrapper code. \$\endgroup\$ – Austin Hastings Jan 5 '18 at 17:44

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.