code style and readability [was: Re: Checking the boolean value of acollection]

[Marco Bizzarri]

class FolderInUse:

def true_for(self, archivefolder):
return any([instance.forbid_to_close(archivefolder) for instance in
self.core.active_outgoing_registration_instances()])

Is this any better? The true_for name does not satisfy me a lot...

well, "true_for" is indeed pretty inscrutable, but I'm not sure that would
be the first thing I'd complain about in that verbose mess...

"verbose mess".

It is always frustrating when you do what you think is your best and
you read that.

Anyway: I'm here to learn, and, of course, part of it is to listen
those who've been there much longer than you.

So, thanks for your sincere evaluation, Fredrik .

(when you pick method names, keep in mind that the reader will see the
context, the instance, and the arguments at the same time as they see the
name. there's no need to use complete sentences; pick short short
descriptive names instead.)

Maybe I'm looking at the wrong direction, right now. From the point of
view of the FolderInUse clients, they will do:

condition = FolderInUse(core)

condition.true_for(folder)

Is this too verbose? This is not a polemic statement, I'm really
asking your opionion.

The expression inside the true_for is indeed complex, and maybe I can
simplify it; however, I'm deeply convinced that

instance.forbid_to_close(folder)

has some good points on it; I mean, once I read this kind of code, I
can hope to understand it without looking at what forbid_to_close
does.

 

[Bruno Desthuilliers]

Larry Bates a écrit :
(snip)

IMHO it reads better if you use the __call__ method of the class to
return the value

IMHO, it makes no sense at all to abuse the __call__ magic method here.

and rewrite it as a regular loop for clarity.
Sometimes the simplest way is the easiest to read.

class FolderInUse:
def __call__(self, archivefolder):
result = False
for instance in self.core.active_outgoing_registration_instances():
if instance.forbid_to_close(archivefolder):
result = True
break

return result

the loop would be simpler with an early return:


for instance in self.core.active_outgoing_registration_instances():
if instance.forbid_to_close(archivefolder):
return True
return False

Then it can be called with:

if FolderInUse(archivefolder):
...

This will call the class constructor and initializer and return a new
instance, not the *instance method* __call__.

 

[Bruno Desthuilliers]

Marco Bizzarri a écrit :

class FolderInUse:

def true_for(self, archivefolder):
return any([instance.forbid_to_close(archivefolder) for instance in
self.core.active_outgoing_registration_instances()])

Is this any better? The true_for name does not satisfy me a lot...

well, "true_for" is indeed pretty inscrutable, but I'm not sure that would
be the first thing I'd complain about in that verbose mess...

"verbose mess".

It is always frustrating when you do what you think is your best and
you read that.

The effbot is sometimes a bit abrupt in his formulations, for sure !-)

 

[Bruno Desthuilliers]

Bruno Desthuilliers a écrit :

Larry Bates a écrit :
(snip)

IMHO, it makes no sense at all to abuse the __call__ magic method here.

Sorry - after a more careful re-read of other posts in the thread, it
might make sense, given the use case :

condition = FolderInUse(core)
if condition.true_for(folder):
# code here


but then, a plain function (or a partial) might be even better - that
is, if the FolderInUse class doesn't have other responsabilities.

 

[Larry Bates]

class FolderInUse:

def true_for(self, archivefolder):
return any([instance.forbid_to_close(archivefolder) for instance in
self.core.active_outgoing_registration_instances()])

Is this any better? The true_for name does not satisfy me a lot...

well, "true_for" is indeed pretty inscrutable, but I'm not sure that would
be the first thing I'd complain about in that verbose mess...

"verbose mess".

It is always frustrating when you do what you think is your best and
you read that.

Anyway: I'm here to learn, and, of course, part of it is to listen
those who've been there much longer than you.

So, thanks for your sincere evaluation, Fredrik .

(when you pick method names, keep in mind that the reader will see the
context, the instance, and the arguments at the same time as they see the
name. there's no need to use complete sentences; pick short short
descriptive names instead.)

Maybe I'm looking at the wrong direction, right now. From the point of
view of the FolderInUse clients, they will do:

condition = FolderInUse(core)

condition.true_for(folder)

Is this too verbose? This is not a polemic statement, I'm really
asking your opionion.

The expression inside the true_for is indeed complex, and maybe I can
simplify it; however, I'm deeply convinced that

instance.forbid_to_close(folder)

has some good points on it; I mean, once I read this kind of code, I
can hope to understand it without looking at what forbid_to_close
does.

</F>

>>> class FolderInUse:
>>>
>>> def true_for(self, archivefolder):
>>> return any([instance.forbid_to_close(archivefolder) for instance in
>>> self.core.active_outgoing_registration_instances()])

IMHO it reads better if you use the __call__ method of the class to return the
value and rewrite it as a regular loop for clarity. Sometimes the simplest way
is the easiest to read.

class FolderInUse:
def __call__(self, archivefolder):
result = False
for instance in self.core.active_outgoing_registration_instances():
if instance.forbid_to_close(archivefolder):
result = True
break

return result


Then it can be called with:

if FolderInUse(archivefolder):
...

-Larry

 

[Bruno Desthuilliers]

Larry Bates a écrit :

Sorry but I respectfully disagree that this is "abuse" of the __call__
method.

As long as we respectfully agree to disagree...

!-)

 

[Bruno Desthuilliers]

Bruno Desthuilliers a écrit :

Larry Bates a écrit : (snip)


As long as we respectfully agree to disagree...

!-)

Anyway, I don't think we have enough background to seriously agree or
disagree on what would make more sense here...

 

[Larry Bates]

Bruno Desthuilliers a écrit :

Sorry - after a more careful re-read of other posts in the thread, it
might make sense, given the use case :

condition = FolderInUse(core)
if condition.true_for(folder):
# code here


but then, a plain function (or a partial) might be even better - that
is, if the FolderInUse class doesn't have other responsabilities.

Sorry but I respectfully disagree that this is "abuse" of the __call__ method.
I do agree that a plain function probably makes more sense but it appears that
the class does "other"things because of references to other class instances, etc.

I also have a personal dislike for early returns because I've found it makes it
harder insert execution trace logging into the code. Just my experience.

-Larry

 

[Fredrik Lundh]

I also have a personal dislike for early returns because I've found it
makes it harder insert execution trace logging into the code.

in a language that makes it trivial to wrap arbitrary callables in
tracing wrappers?

</F>