1

I am a hobbyist programmer, working on a much more complex Python project than I've attempted before, which is in the form of a Python library.

I find that I'm often passing **kwargs around (as described in this question), but I'm also realising it's bad for readability and maintainability. The answer to the previous question suggests that the code may need to be refactored, but I am unsure how to do so while still achieving what I was trying to achieve in the first place.

Here's an example of what I find myself doing. My application is built around Shape objects that can be displayed on screen. Displaying them is quite a complex task, so I have a DisplayManager class that looks like this:

# displayManager.py

class DisplayManager:
    def __init__(self):
        # set up display context, open a window, etc.
    def display_shape(self, shape, color, opacity=1):
        # query the Shape object for the data needed
        # to display it, then process that data and
        # show it on screen

It makes sense to put the display method here rather than in the Shape class, because the code and libraries used for displaying are quite distinct from the rest of the application's logic, so I wanted to keep it separate.

But most of the time the user shouldn't have to worry about the displayManager class. So I end up having a global displayManager object and putting a convenience display method in Shape, like this:

# shape.py
import display

class Shape:
    # ...
    def display(self, color, opacity=1):
        display.globalDisplayManager.display_shape(self, opacity)

The problem is that now I have the arguments color and opacity in two different places in the code, in separate files, which violates DRY. It's quite likely that these arguments or their default values will change in the future, and I would have to remember to update them in both places. So instead, I end up doing this:

# shape.py
import display

class Shape:
    # ...
    def display(self, *args, **kwargs):
        display.globalDisplayManager.display_shape(self, *args, **kwargs)

Now the arguments are specified in only one place, and if I decide to change them or add new ones I only have to do it in one place, in display.py where the display routines are implemented.

The problem is that it's not very good from the user's point of view. Looking at Shape.display doesn't actually tell you its arguments, and to find that out you have to delve into the DisplayManager class, which I didn't want the user to worry about.

Of course this can be resolved by describing the arguments in the docstring for Shape.display, but then I'd have an even worse violation of DRY, since the documentation would be in a different place from the implementation and it would be easy to forget to keep in them in sync.

So after that long description, my question is how (or whether) I can achieve my goals without this big disadvantage? That is, how can I keep the display implementation separate from the Shape implementation, in such a way that (i) I don't have to remember to update multiple files if I change the API, and (ii) the user doesn't have to dig into the implementation to find out how to use it? More broadly, if I find myself passing *args and **kwargs around like this, what should I be considering instead?

(Note: a possible response to this question is that I shouldn't be changing the API all the time anyway. My reply is that at this stage the only user is me. The API will all be locked down before I release, if I ever do release, but for now it's important to keep it flexible so I can figure out what works.)

2

Dividing responsibilities between classes is tricky since there's no right answer and it'll take future usage patterns to find out what makes for better answers. Anyway it's inevitable that the classes will call on each other, each handling its own responsibility and passing the buck -- and several argument values.

Yes, adding features like texture and line style will require adding arguments to some methods, or maybe just adding methods like set_texture(). Similarly, color could be an argument or a property of the shape or of the canvas.

The least DRY part of your example is the default argument values. Well, the default opacity 1 is unlikely to change, but you could define a constant for each default value and use that in all relevant method definitions in all the classes.

Interfaces will evolve. In fact, the great thing about interfaces is they're easy to evolve -- although if they have unknown clients out in the world, one should stick with compatible changes. Meanwhile, a refactoring editor such as PyCharm makes it easier to make consistent method signature changes across multiple files including their docstrings (although this feature doesn't work as smoothly in Python as in Java, for want of the type declarations).

You are right that *args, **kwargs is unhelpful to clients. It's also unhelpful to code inspection tools and documentation tools, such as asking PyCharm to display a method's parameters when you're coding a call to it. I'd prioritize these issues over the DRY concern.

There are different kinds of "DRY violations," with different severities. I suggest not worrying overly much about that design guideline.

If your project is a graphics package, i.e. that's not just your example for this question, then a good tip is to look at various graphics APIs to see how they addressed the same design question.

  • Thank you, this gives me some things to think about. I didn't mean to invoke DRY as an unthinking absolute, it's more that I'd found myself having to make changes in multiple files and started using the **kwargs trick to avoid that, and only later realised it might not be a good idea. (Is isn't a graphics package as such, it's more like a CAD library. Its main predecessor has its own purely functional programming language, which is very different from Python, making it less helpful to imitate its API. The example is more or less a real one from my code, but there are others as well.) – Nathaniel Dec 31 '18 at 6:26
2

Semantic Fix

Why is that the users should not be aware of this display?

I feel that this is the core issue. What precisely is a Display, and will there be nuanced versions of it? Say a display for the screen, and a display for a printer? In such a case only the using code will known which to select.

Even if there is only one implementation, you have already pointed out that Shape has no coupling to Display, so you are keeping them separate. Yet the immediate solution is to couple the shape to the display? This will make it hard to make changes later.

The best option would be to keep that separation of concern. If you must couple them somehow, consider a Scene object which holds a collection of Shapes, and a reference to a Display. It would then be ideally suited to help optimise draws, and perhaps identifying the exact shape the mouse is over. This will simplify the management of many drawing related concerns.

Syntatic Fix

You will need define another value as representing the default, and mapping that to the real default later:

class Shape:
    # ...
    def display(self, color, opacity="default"):
        display.globalDisplayManager.display_shape(self, color, opacity)


class SomeDisplayManager:
    # ...
    def display(self, color, opacity="default"):
        # provide the real default...
        if opacity == "default":
            opacity = 1         
        #do stuff
  • Thank you. You are probably right about the relationship between Display and Shape. I guess my reasoning was that some of the intended audience will not have much programming experience, and myShape.display("red") looks much less scary and easier to explain than display.displayManager.display_shape(myShape, "red"). It's also less typing and looks neater. But this might be a kind of premature optimisation. The latter, though more complicated looking, is in fact more logical. I will have to think about this. – Nathaniel Dec 31 '18 at 6:04
  • @Nathaniel Why could you not simply have display(myshape, "red")? Python does allow for global functions. This can be overloaded to have a version which also accepts a Display and another version which does not but forwards the global to the overload accepting a Display. If you also pass other types to display, that is fine. Python does allow you to clearly specify the argument type. – Kain0_0 Dec 31 '18 at 6:34
  • That's probably a better solution. I was thinking about it after posting my comment, but it just hadn't occurred to me before that. I have to think a bit about how all this translates to the other instances of this issue in my code. – Nathaniel Dec 31 '18 at 6:39
  • 2
    @Kain0_0 I think you misunderstood Python function overloading. A class can only implement one method with a particular name. (It doesn't complain about a duplicate definition, but the second one replaces the first.) The webpage you linked to defines a method with an optional argument, like @Nathaniel's opacity=1 argument. Your point could be implemented as def display(self, color, opacity=None) in both Shape and DisplayManager, with the latter using an if statement to translate None to 1, so only the latter defines what the option defaults to. – Jerry101 Dec 31 '18 at 7:42
  • Just to note also, I followed up on the link about type checking. As of now (end of 2018) it's highly experimental - the syntax is accepted but the interpreter just ignores it. You have to run an external program (mypy) to actually do the type checking, which is "still in development" and only supports "most" Python features. So it could be a great solution in the future but as of now it's not fully implemented and I wouldn't rely on it. – Nathaniel Dec 31 '18 at 8:40
1

**kwargs when used this way has a few downsides - first, you're puncturing through encapsulation of your components, so everything knows about everything else, and second, by having a call stack full of undefined variables, you open yourself up to some very confusing bugs where one call uses a variable that you meant to pass further down the stack. Last but not least, it's a nightmare to document it, because everything in your call stack has a potential argument for everything else.

For what you're doing, there are a couple good ways to fix this:

  • Use object state. Add color and opacity as properties of Shape, and reference them when needed. The code defining the shape now sets those fields before calling display. This strengthens the encapsulation of the program, as your manager call doesn't need to be told what the color and opacity are

  • Use an object named something like DisplayParameters and pass it into the shape's call to display. This will make clear to the caller what information they passed in is going to go where, and gives a clear hook for documentation of what you're passing in.

  • Consider keeping these two parts decoupled. It looks like you've got a global that you're using so your convenience functions can get a reference to a DisplayManager. Without a good way to swap out or mock that DisplayManager, you're going to have a hard time testing those functions, and you might not be saving yourself that much client code for the trouble that this coupling gets you into.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.