1

As a minimal example, consider the following (in Python):

class Foo:
    def __init__(self, bar):
        # Assume bar is a valid at this point
        self.bar = bar

    def divide_by_bar(self, num):
        return num / self.bar

If the attribute bar is public, my understanding is that it is valid for the client to do something like:

>>> foo = Foo(1)
>>> foo.bar = 0
>>> foo.divide_by_bar(2)
Traceback (most recent call last):
   ...
ZeroDivisionError: division by zero

We encountered a more complex version of this in our team and I suggested to make the attribute private, or read-only (or to consider the handling the exception). However, I got the reply that, since it is only used in one place in our code base, it is fine to leave it like that. I agreed to that, but I have noticed numerous examples of this practice in our code base and I am uncertain when should one be more strict with these cases. Should I ignore it until it is used in multiple places?

pob
  • 109
  • 6
  • In general it is better to start with all attributes private, and only expose when necessary. There's a suite of subtle bugs when you start inverting your logic. Of course in this particular situation you probably should do some boundary checking. If it is illegal for `bar` to be 0, then check for that when assigning to bar. If it is legal, then check for and handle the case of bar=0 in divide_by_bar(). – Berin Loritsch Jul 11 '19 at 18:11
  • Related: "[Why do we have mutator methods?](https://softwareengineering.stackexchange.com/q/347293/9457)", "[Does Python have any features which can be used for encapsulating private data?](https://softwareengineering.stackexchange.com/q/303021/9457)", "[When or why should one use getters/setters for class properties instead of simply making them public properties?](https://softwareengineering.stackexchange.com/q/144347/9457)" – outis Jul 12 '19 at 17:25

3 Answers3

2

Python has no good language-level encapsulation mechanisms, so in a way this doesn't matter. But Python linters do recognize the “leading underscores mean private” convention, so respecting this encapsulation can be enforced.

Here, just adding an underscore will make the intention of the code much clearer. You shouldn't necessarily go and seek out any such code snippets to fix them, but if you do come across such an example, then improving it would be a no-brainer.

Alternatively, Python offers some mechanisms to make objects read-only. I like the attrs module for easily creating immutable classes, and with Python 3.7 you could also use dataclasses. Other approaches are probably not worth the effort, e.g. namedtuples have weird syntax and do stuff that you might not want (like making objects comparable).

Although the code could be improved (by clearly marking private fields as private, or by making the object immutable), this kind of Python code is not uncommon. If you want a safe programming language then Python is not for you. A lot of stuff in Python is merely a convention, and everyone should respect that convention. What is part of the public interface is not necessarily defined by a naming convention or by some real encapsulation mechanism, but often by what is mentioned in the documentation.

amon
  • 132,749
  • 27
  • 279
  • 375
  • I agree that one should not go hunting for these cases, but in this case it was encountered during a code review. Let's assume that the intention of making the attribute public is clear (no underscore, part of the docs, confirmed by the author). Is this correct because only a single client is using it in a reasonable way (around the lines "we are consenting adults") or should the "correctness" be evaluated independent of it existing usage? – pob Jul 11 '19 at 19:40
  • 2
    `Is this correct because only a single client is using it in a reasonable way ` No! Well. maybe I'm biased by OOP but, such a statement sounds an [normalization of the deviance](https://en.wikibooks.org/wiki/Professionalism/Diane_Vaughan_and_the_normalization_of_deviance) to me. This is something you want to avoid absolutely. It's like the test that fails randomly, you know why but you think it's not that important. Eventually, someone in the future thinks that failing tests are not so bad, If it's allowed to happen once, why not twice? – Laiv Jul 12 '19 at 06:56
2

As a general rule, attributes of a class should be part of the public interface if and only if it makes sense within the contract of that class that callers would have access to that attribute.

If not all the values that can be assigned to the attribute are valid within the invariants that are supposed to hold for the class, then it might be better to not expose the attribute directly but only via a property (or an getter/setter pair of functions), so that any attempt to set an invalid value can be caught early.
The same holds if the outside world should only have read-only access to the attribute.

I got the reply that, since it is only used in one place in our code base, it is fine to leave it like that.

This I find a very dangerous attitude. This easily leads to arguments like "it was done like that before, so it must be acceptable to do it like that again" and before you know it you have to officially support the usages that were deemed inappropriate but innocent at first. That may severely hinder your possibilities to do maintenance later.

for that reason, you should push back on that one usage and even try to avoid the possibility that such one usage can creep in.

Bart van Ingen Schenau
  • 71,712
  • 20
  • 110
  • 179
1

I am uncertain when should one be more strict with these cases

I would argue that one should be more strict when it's important to be more strict.

In your example you end up with a divide by zero problem. Is that bad? If this is an internal tool and the cause of the error is obvious because its only used by developers, then it's probably fine the way it is. If this is code for a nuclear reactor or medical equipment, maybe it's time to be a bit more strict.

You listed a couple of options, but you seem to have left out the option of creating a setter that could catch such an error. You can make bar public while still protecting against bad inputs.

Bryan Oakley
  • 25,192
  • 5
  • 64
  • 89
  • It is part of production code on a very expensive device. However, this interface is only exposed to other developers, which is why I hesitate if I should care at all. You are completely right in that I left out that (and maybe other) options, but my question goes more in the direction if this should be pointed out in the first place. – pob Jul 11 '19 at 20:14
  • @pob “this interface is only exposed to other developers, which is why I hesitate if I should care at all” – developers are not necessarily extra careful. Discouraging direct writes to this variable can help detect problems earlier. This isn't directly about safety, more about getting quick feedback when there are problems. Similar to why unit tests are helpful: they do not guarantee your code is correct, but they'll likely alert you to problems. – amon Jul 11 '19 at 21:15