Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 11:33

Outside of these comments, the code looks neat and Pythonic. [1]: http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form https://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form

Outside of these comments, the code looks neat and Pythonic. [1]: http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form

Outside of these comments, the code looks neat and Pythonic. [1]: https://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form

Source Link

answered May 20, 2014 at 15:11

BeetDemGuise

answered May 20, 2014 at 15:11

BeetDemGuise

4.2k
12
29

I like your code: its quite interesting. While reviewing, I was hoping to see if there was a better way if keeping track of the instances of each ObservableMethod or if we could combine the ObservableMethod and ObservableMethodDescriptor classes. However, I could not think of a better way to store the instances of each ObservableMethod.

With that being said, I have a few recommendations, mostly centered around naming:

Rename your event function and Cleanup class.

Currently event is quite generic. Plus, it doesn't follow the 'function-names-start-with-verbs' convention. I would recommend renaming it to make_observable. This conveys much better what it (and its decorator) actually does.

As for renaming Cleanup: because of its verb-based name, it feels like a function. Classes are things, thus it makes sense to have a noun-based name; maybe CleanupHandler.
Improve your variable names.

Currently you have several 1-letter or 2-letter variable names. Make those names more descriptive:
```
def __get__(self, inst, cls):
 . . . 
 if ID in self.instances:
 # World record, organic matter?
 wr, om = self.instances[ID]
```
Yes, in context we can deduce the meaning of the variable names. However, it pays to be more explicit rather than trusting that everyone understands what is going on:
```
def __get__(self, inst, cls):
 . . . 
 if ID in self.instances:
 # Much better.
 weak_ref, observable_method = self.instances[ID]
```
In the example above is another point I want to make: ID is not constant (as convention says its name suggests). I would recommend renaming it to obj_id or something of the like. You could just use id but that may be a little confusing (and possibly dangerous) since id is a basic Python function.

My final point about variable names deals with multiple word names. You used 'camelCase' in your code. Pythonic convention say that underscores_in_names is preferred.

Spacing

Insert blank lines to help group logical sections of code. Looking at your __get__ code, inserting blank lines helps the visual flow of the method:

def __get__(self, inst, cls):
 if inst is None:
 return self
 ID = id(inst)
 if ID in self.instances:
 wr, om = self.instances[ID]
 if not wr():
 msg = "Object id %d should have been cleaned up"%(ID,)
 raise RuntimeError(msg)
 else:
 wr = weakref.ref(inst, Cleanup(ID, self.instances))
 om = ObservableMethod(inst, self._func)
 self.instances[ID] = (wr, om) 
 return om

format() vs. %-Notation

[As this link says][1], %-notation isn't becoming obsolete. However, it says that using format() is the preferred method, especially if you are concerned about future compatibility.
```
# Your way
>>> print 'Hello %s!'%('Darin',)
# New way
>>> print 'Hello {}!'.format('Darin')
```
This also saves you from having to create temporary tuples just to print information.

Outside of these comments, the code looks neat and Pythonic. [1]: http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form

lang-py