Re: [RFA]: BOOL coding standards (Was: Problem with +numberWithBool: ?)

[Sheldon Gill]

> if ([myObject doSomething]) ...
>
> which is immediately understandable at a glance. But when I have a
> misbehaving program, I often wonder, is it really doing what I think
> it's doing?

All of this touches things which I feel are quite important. Let me start out 
by clearly declaring
1) I'm a big fan of the principles behind "Design By Contract".
2) I firmly believe in being assert()ive

In regards to this discussion, it raises the point

* Say what you mean and mean what you say.

ObjC, following from Smalltalk, sends messages to objects. Those messages here 
are written in English giving rise to some quite literate programming. Caveat 
is that the writer needs to be literate.

So considering your example, Adam, I know it's supposed to be generic but...

doSomething for a test condition is an invitation to problems, because 'do' is 
a verb clause. Let's make the discussion a little more concrete by 
considering this example:

if ([aValve checkStatus])

it matches the 'doSomething' approach but is actually confusing because it 
fails to be explicit. You're obviously checking the status of the valve but 
what does the return mean? Does 'No' mean something about the current status 
of the valve or does it mean something about the check process itself?

I feel a much better form is a possessive clause like
if([aValve isOperable])
if([aValve hasPower])
if([aValue isCommunicating])

because these have fairly explicit meanings for which the truth value is 
reasonably apparent. This becomes very important when the required logic 
becomes necessarily complex and involved. (think Karnaugh maps...)

So when developers write:

if( [] == YES)

what they generally mean is

if( isTrue() )

in the Boolean algebra sense.  Confusion arises because of the 
_implementation_ of testing in 'C'.  Thanks to it's origins (and I'll happily 
rant on that for many pages) the way if() works comes from testing the 
processor 'Z' flag and branching. So those writing code discovered that 
anything non-zero wouldn't set the Z flag...  Not a real language design 
decision. It arises from practice, not theory {cue Dijkstra}

but what we have here is just one of many such cases in testing. Let me 
highlight another one dear to my heart in C:

linked_list *ptr;

if ( ptr != NULL )

when what you really mean is

if ( isValidPtr(ptr) )

or better yet

if ( isValidPtrToLinkedList(ptr) )

because it's not just that you don't want the pointer value to be non-zero.

Although it's more verbose, what you really mean is much more apparent. This 
is invaluable when it comes to debugging because it allows you to create 
functions for the above to check that what you expect really is the case and 
it allows other developers to know what you were trying to achieve.

Contrast this to the other situation in C where

if ( ptr != NULL )

is supposed to mean

if ( haveArgument(ptr ) )

All too often I see code where it checks for a non-zero pointer, simply 
returning otherwise, without considering whether a zero input is meaningful. 
In many cases, it's not and only serves to obscure/propagate an error which 
could have been easily and quickly picked up by throwing an exception.

So, within GNUstep code I think the _right_ thing to do is precisely Richard's 
recommendation of

isYES(x): IF  x NOT IN {YES|NO} THEN raise

which we can use to test all "+/- (BOOL)aMethod".  Any method which returns 
something other than {YES|NO} is bad practice we should catch and eliminate.
Any method which is defined to return one of these two values but actually can 
return additional values is out of specification and should be corrected.

I also think that this approach should be adopted more generally.

From my own experience
- it highlights mistakes in specifications
- it catches mismatch between specification and implementation
- it catches bad assumptions
- it catches errors I might otherwise have missed

Finally, to Helge's example from OGo:

Well, that's right or wrong depending on the methods' definition. Consider:

- (BOOL)isEqual: anObject

the right thing here is "return [self compare:obj] != 0 ? NO : YES;" because 
you've stated that the return is conceptually BOOLEAN (one of two values).
The method
- (int)compare: anObject
by definition returns one of _three_ possibilities and so has a return type to 
suit. Indeed, that's also why it is called something like "compare" rather 
than "isGreaterThan:".

If the calling code used the proposed "isYes()" then you would have easily 
identified the issue during development. Nice and early when correction is 
cheap :)

Conversely, if the definition was like
- (int)compareTo: anObject

then the calling code is at fault if it's explicitly testing against either NO 
or YES because that's not what the method is about!

In light of the previous paragraphs, "compare" is another verb clause so 
writing
if ([anObject compareTo: anotherObject])
i
s poor form. One better way would be
if ([anObject isEqualTo: anotherObject])

if that is what you mean. You could also use a functional approach:
if ( isEqual(anObject, anotherObject) )

Comparison also applies to two different concepts: equality and equivalence. 
Which is meant is not apparent from "compare". Isn't English wonderful?

BTW, #define is the old C way to do enum {} because they didn't exist...


Coding Standards should explain good practice and approach. I'd really like to 
see the standard enhanced more completely along this line to explain what is 
good and what is bad and why this is thought so.

My apologies if this seems a little long winded. I wanted to be fairly clear 
and reasonable in my case first up, rather than extending the thread with 
many questions and clarifications.


Regards,
Sheldon