Readability: Do not abuse classes as global state

July 8, 2013 ·
About 3 minutes

You know that passing state around different functions by using global variables is bad: it results in spaghetti code, it introduces side-effects to your functions and, well, is just bad practice. Then: don't make the same mistake when using classes.

The form this manifests in code is by having a particular class method (not necessarily the constructor!) initializing a member field and later having other unrelated methods querying the attribute "out of the blue".

Consider the following code in which we have the definition of a class to represent a real-world object and a caller test method:

Yes, I've seen something similar in production. No, this is not good. There are many problems with this design, but let's focus on the one that is the point of this article.

The specific problem I want to highlight is in the query_dimensions() method: such method is abusing the local attributes as input parameters and is also storing its various return values into attributes. The fact that this method is public is even worse, as illustrated by the need to call it before being able to access public attributes.

Conceptually, a function to retrieve values from a database should be receiving the database and the object identifier as parameters, and return the results of the query. We want the function to not use global state and instead look like this:

Note that this function has no hidden dependencies: all data read or written is accessed via input parameters or returned via return values.

Now, plugging this into our previous class allows us to "fix" the volume() method to not rely on updating the local variables in a magic manner. However, having changed the function this way exposes that the various public attributes cannot now be accessed from outside. We need to rewrite the code quite heavily:

This new code is certainly much longer than the original code. However, this updated version is easier to follow because the points at which attributes get updated have been made explicit. Also, this offers better encapsulation by avoiding the caller to know how to recompute internal values.

To summarize: don't use attributes for data that only need to be passed around methods. Attempt to make your methods independent from the instance, or at least think about them this way so that you realize what dependencies they have.