object.enable() anti-pattern

[André Malo]

Another example is running a subprocess in Unix-like systems.

fork()
open/close file descriptors, set limits, etc
exec*()

For running a subprocess, only fork() is needed. For starting another
executable, only exec() is needed. For running the new executable in a
subprocess fork() and exec() are needed. I think, that's a bad example.
These APIs are actually well-designed.

nd
--
Gefunden auf einer "Webdesigner"-Seite:

Programmierung in HTML, XML, WML, CGI, FLASH <

# André Malo # http://pub.perlig.de/ #

 

[Chris Angelico]

For running a subprocess, only fork() is needed. For starting another
executable, only exec() is needed. For running the new executable in a
subprocess fork() and exec() are needed. I think, that's a bad example.
These APIs are actually well-designed.

That said, though, there's certainly plenty of room for one-call APIs
like popen. For the simple case where you want to start a process with
some other executable, wait for it to finish, and then work with its
return value, it makes sense to hide the details of fork/exec/wait -
especially since that simple API can be cross-platform, where fork()
itself is quite hard to implement on Windows.

ChrisA

 

[Dennis Lee Bieber]

But even if C allowed you to do so, doesn't mean that it is a good idea.
At least some variants of Pascal force you to do the following:

# Pseudo-code.
f = open(filename)
really_open(f)
data = read(f) # or write, or *any other operation at all*

Surely we can agree that having to call both open() and really_open()
before you get an actually opened file that you can use is one call too
many? There is *nothing* you can do with f before calling really_open().
So why require it?

Well... As I recall, original Pascal required all files to be named
on the command line invocation. There was no way to dynamically assign a
file name to a file object within the language.

{Looking at the 3rd Edition Jensen&Wirth confirms that -- even the ISO
standard Pascal does not have an internal way to link a filename to a
file variable.}

(For the record, "really_open" is spelled "reset" or "rewrite" depending
on whether you want to read or write to the file.)

So your example, the open could better be names "assign" or "link",
as it associates the name of the file with the file variable, but does
not do anything with the file system itself -- that would be the
standard Pascal reset/rewrite.

A concept that is probably an artifact of the mainframe operating
systems in those days. Linkage of a physical file to a languages
internal "file" concept tended to be done with job control language
surrounding the invocation of the program itself.

 

[Dennis Lee Bieber]

Sure. I can serialize a path name. I can't serialize an open file
descriptor.

I really need to pull a few books out of storage... My Amiga RKMs,
and my LS-DOS 6 "The Source" and programming manuals. As I recall, the
LS-DOS system calls for I/O crossed your line about serialization...

The file control block of an open file contained the information
about record length (from the directory), mode, and current position. If
one closed the file, the FCB contained the filename <G>

Granted, that system used a single layer directory (but did have a
utility that allowed for "partitioned data sets" -- a way of storing
multiple files within one larger file; most of the system libraries were
implemented that way).

 

[Robert Kern]

But isn't that the case for all anti-patterns?

We agree that GOTO is an anti-pattern. That doesn't mean that there
aren't valid reasons for using GOTO. Sometimes there are good use-cases
for GOTO that outweigh the harm. By calling it an anti-pattern, though,
we shift the onus onto the person wanting to use GOTO: justify why you
need it, or use something else.

Yes, that was the point I was making. You seemed to be defining away the
legitimate instances as not instances of the pattern at all because they were
legitimate, and that appeared to me to be defeating the purpose of having the
discussion.

On a related note, I *don't* think it's a good idea to phrase it as "justify why
you need it". I don't think that gives very good guidance to a novice when they
are given the task of designing something. People can come up with a
justification for just about anything, especially when they are only justifying
things to themselves. I think it's more important to just talk about the
situations where a pattern is useful, and the common situations where people,
for whatever reason, *think* that a pattern is useful, but isn't. Naming it a
Pattern or Anti-pattern is really just a measure of how bad the latter half of
that is compared to the first half, and is less interesting than the discussion
itself. That's why I had a bug up my ass about what looked like the exclusion of
the "good" uses. It's the good examples that give novices an idea of what a good
justification looks like, so they can tell if the justification they are giving
themselves measures up.

Would you object less if I called it a "code smell" than an "anti-
pattern"? If so, I accept your criticism, and call it a code smell: the
less temporal coupling your API has, the better.

That was not really my objection. I was objecting to the way you appeared to be
defining the particular pattern in question. But as we appear to agree on the
important matters, I won't press it further.

How about this?

http://legacy.thecodewhisperer.com/post/366626867/improving-clarity-by-removing-temporal-coupling

There's something about Java mixedCase that makes my eyes glaze, so I'll take
your word for it.

Another example of temporal coupling is json_decode in PHP: you must
follow it by a call to json_last_error, otherwise you have no way of
telling whether the json_decode function succeeded or not.

http://php.net/manual/en/function.json-last-error.php

I suspect that the author might say something about error checking being
optional. But yeah, terrible API.

--
Robert Kern

"I have come to believe that the whole world is an enigma, a harmless enigma
that is made terrible by our own mad attempt to interpret it as though it had
an underlying truth."
-- Umberto Eco

 

[Chris Angelico]

I suspect that the author might say something about error checking being
optional. But yeah, terrible API.

The problem with that one isn't that error checking is optional, but
that errors are signalled with a perfectly legal return value (FALSE,
if I recall correctly - which is also returned if you json_decode a
boolean false). Better design would either throw an exception on
error, or have a unique sentinel representing the errorness of the
return value.

ChrisA

 

[Wayne Werner]

I'd be curious to see in-the-wild instances of the anti-pattern that you are
talking about, then. I think everyone agrees that entirely unmotivated
"enable" methods should be avoided, but I have my doubts that they come up
very often. Do programmers have a natural tendency to make an extra,
completely unnecessary method? I would think that they have a natural
tendency to the opposite.

In my experience, everyone has a reason in mind when they follow a
pattern/anti-pattern. It is pretty rare that someone just does some specific,
nameable thing for no reason at all. There is no need to call out an
anti-pattern for which no one has a reason to do it. But there is a continuum
of reasons. Some reasons are better than others. Some reasons only apply in a
small set of circumstances but seem like they would apply more generally, at
least to novice programmers. Programmers can be wrong about what they think
the (anti-)pattern actually achieves. The whole point of naming an
anti-pattern is to discuss those reasons, show where they are misapplied,
where YAGNI, why novices overuse it, other patterns that should be used
instead, and also the circumstances where it is actually a good pattern
instead.

I'll share the anti-pattern that I've seen many times (not actually in
Python)

class CoolPresenter:
def __init__(self):
self.view = None
self.some_property = None
self.other_property = None

def initialize(self):
self.view.disable()
data = self.load_data()
self.view.data = data
self.view.enable()


def reload(self):
if self.view is None:
raise NotInitializedError("Error: Please setup class")
self.view.disable()
data = self.load_data()
self.view.data = data
self.view.enable()



Then you would see code like this:

presenter = CoolPresenter()
presenter.view = CoolView()

This is just plain silly for a few reasons:

- It's ambiguous. I don't know what's required for the CoolPresenter
to function properly.

- The temporal coupling mentioned earlier. I can create an instance of
a class and then call a function (say `reload`) and then boom! My
program crashes. There is *no possible* use case of this class where
you can use it without a view.


The motivation behind this anti-pattern that I've seen is the desire to
not have a constructor that "does too much". So you end out with an empty
constructor and temporal coupling, and a terrible API that doesn't clearly
explain the requirements of the class. Your class constructor should
*require* everything that is necessary to have a stable state when the
class is created (i.e. you should be able to properly call any function,
set any property without an exception happening)

Why? Less bugs, easier to comprehend, change/update your code. Easier to
use the class.

-W

 

[Wayne Werner]

That seems like an overly broad statement. What
do you think the following should do?

f = open("myfile.dat")
f.close()
data = f.read()

To clarify - you don't want a class that has functions that need to be
called in a certain order with *valid input* in order to not crash.

Exactly what does happen - a ValueError is raised because you're(*)
passing self into the file.read() function, and that input is invalid
input - specifically:

ValueError: I/O operation on closed file

*where you actually means python, because when you call
`your_instance.method()`, it works effectively like a call to
`YourClass.method(your_instance)`

-W

 

[Terry Jan Reedy]

To clarify - you don't want a class that has functions that need to be
called in a certain order with *valid input* in order to not crash.

Exactly what does happen - a ValueError is raised because you're(*)
passing self into the file.read() function, and that input is invalid
input - specifically:

ValueError: I/O operation on closed file

*where you actually means python, because when you call
`your_instance.method()`, it works effectively like a call to
`YourClass.method(your_instance)`

The new idiom
with open("myfile.dat") as f:
data = f.read()

partially encapsulates operations on the opened file before the
automatic close. It is true however, that after the with statement, f in
still uselessly bound to the closed file. (Well, you can check the mode,
encoding, and other attributes, so not totally useless.) To completely
encapsulate, one can end the block with 'del f'. I checked and this
works. Since the external 'as x' name binding is optional, an internal
reference to the context manager (the io.xxx instance) is kept in order
to call the __exit__ method, and that is not affected by deleting the
optional visible binding.

tjr

 

[Terry Jan Reedy]

I'll share the anti-pattern that I've seen many times (not actually in
Python)

class CoolPresenter:
def __init__(self):
self.view = None
self.some_property = None
self.other_property = None

def initialize(self):
self.view.disable()
data = self.load_data()
self.view.data = data
self.view.enable()


def reload(self):
if self.view is None:
raise NotInitializedError("Error: Please setup class")
self.view.disable()
data = self.load_data()
self.view.data = data
self.view.enable()



Then you would see code like this:

presenter = CoolPresenter()
presenter.view = CoolView()

This is just plain silly for a few reasons:

- It's ambiguous. I don't know what's required for the CoolPresenter
to function properly.

- The temporal coupling mentioned earlier. I can create an instance of
a class and then call a function (say `reload`) and then boom! My
program crashes. There is *no possible* use case of this class where
you can use it without a view.

Thank you for this examples. It makes Steven's point clearer than the
'file object' example. The problem with the latter is that objectors
could could point to file path objects, which are used to do some
manipulations of files and directory entries on the storage device
*without looking at the specific file data*. Such file data, as opposed
to directory data, never enters the program data space and with smart
enough devices, need not enter the CPU and its RAM. They could then
confuse them with file access objects which are used to transfer
specific data *between* files and program data space. The confusion is
adied by the fact that file path objects are used to create file access
objects. They do separate jobs and it seems that they are well kept
separate, even if there are languages where file path objects can have
file access functions turned on and off.

 

[Greg Ewing]

To clarify - you don't want a class that has functions that need to be
called in a certain order with *valid input* in order to not crash.

Exactly what does happen - a ValueError is raised because you're(*)
passing self into the file.read() function, and that input is invalid

The same argument can be applied to:

foo = Foo()
foo.do_something()
foo.enable() # should have done this first

You're passing an invalid input to Foo.do_something,
namely a Foo that hasn't been enabled yet.

 

[Fábio Santos]

passing self into the file.read() function, and that input is invalid

The same argument can be applied to:

foo = Foo()
foo.do_something()
foo.enable() # should have done this first

You're passing an invalid input to Foo.do_something,
namely a Foo that hasn't been enabled yet.

I don't think you can really count that as invalid input in OOP terms.
After all in most languages `self` / `this` / whatever is not an argument
to every method.

 

[Chris Angelico]

I don't think you can really count that as invalid input in OOP terms. After
all in most languages `self` / `this` / whatever is not an argument to every
method.

Yes, it is; it's just often implicit. C++ lets you poke around with
the internals, and it's pretty clear that 'this' is an argument. (See
for instance what happens with the gcc 'format' attribute - I can't
find a convenient docs page, but it's been mentioned on SO [1] and can
be easily verified.) EMCAScript lets you call any function with any
'this' by using the .call() or .apply() methods - which, in my
extremely not-humble opinionated opinion, is bad design (closures work
implicitly, but the 'this' pointer doesn't??). Python turns an
attribute lookup on an instance into an attribute lookup on the class
plus a currying. One way or another, the bit-before-the-dot is an
argument to the function.

[1] http://stackoverflow.com/questions/...se-attribute-format-printf-x-y-inside-a-class

ChrisA

 

[Fábio Santos]

I don't think you can really count that as invalid input in OOP terms. After
all in most languages `self` / `this` / whatever is not an argument to every
method.

Yes, it is; it's just often implicit. C++ lets you poke around with
the internals, and it's pretty clear that 'this' is an argument. (See
for instance what happens with the gcc 'format' attribute - I can't
find a convenient docs page, but it's been mentioned on SO [1] and can
be easily verified.) EMCAScript lets you call any function with any
'this' by using the .call() or .apply() methods - which, in my
extremely not-humble opinionated opinion, is bad design (closures work
implicitly, but the 'this' pointer doesn't??). Python turns an
attribute lookup on an instance into an attribute lookup on the class
plus a currying. One way or another, the bit-before-the-dot is an
argument to the function.

[1] http://stackoverflow.com/questions/...se-attribute-format-printf-x-y-inside-a-class

ChrisA

I know ECMAScript does that. It would be nice to be able to pass an
instance method as a callback argument without using `.bind(theInstance)`.

At any rate, exposed or not, that is all still internals. Exposing the
ability to set the ` this ` as an argument is, I think, a functional
feature (map(str.strip, file) is a good example)

That said, I didn't know c++ did that, but it makes sense with what I read
somewhere about c++ starting out as a transcompiler-to-c-based language.

 

[Wayne Werner]

The same argument can be applied to:

foo = Foo()
foo.do_something()
foo.enable() # should have done this first

You're passing an invalid input to Foo.do_something,
namely a Foo that hasn't been enabled yet.

That is the crux of the argument - as designer of the class *you* need to
ensure that when your constructor is done, your class is in a stable
state. And that every other state transition (with valid input) results in
your class then being in a stable state.


If anything, the stronger argument is that `file.close()` is not a well
designed function because it leaves your object in an unstable state.

Which I would be inclined to agree with, but I couldn't give you the
answer for what makes it better. Because the answer is the best one you
can get in computer science: It depends.


The reason that it depends, is because it depends on what you want to do.
Do you want a program that seems purely functional? Do you want a program
that's easy to maintain? Do you want a program that more accurately models
the "real world"?

Personally, I think the file object API in Python is about as good as it
can get - but that's because it's working with "physical" things (i.e.
files - bits on a platter, or flash/SSD drive...) which necessarily have a
temporal nature. And it's much less badness to blow up on a call to `read`
than it is to remove the `read` function and die with a NameError when the
underlying file is in a closed state.


At least in my opinion
-W