> Urk.  First off, let's consider a simple Composite example,
> *not* involving any additions to Object, because the whole point of
> this thread is "what may legitimately go in Object".
>
> So we have
>     Thingy
>     >>isComposite ^false
>     >>childrenDo: aBlock ^self
>     >>descendantsDo: aBlock
>         aBlock value: self.
> self childrenDo: [:each | each descendantsDo: aBlock].
>     >>allChildrenSatisfy: aBlock
>         self childrenDo: [:each |
>     (aBlock value: each) ifFalse: [^false]].
> ^true
>     >>allDescendantsSatisfy: aBlock
> self descendantsDo: [:each |
>     (aBlock value: each) ifFalse: [^false]].
>         ^true
>     subclass RedThingy
>     subclass BlueThingy
>     subclass GreenThingy
>     subclass CompositeThingy 'children'
>        >>isComposite ^true
>        >>addChild: aThingy ^children add: aThingy
>        >>removeChild: aThingy ^children remove: aThing
>        >>childrenDo: aBlock children do: aBlock
>
> Now, the the question for Andres Valloud is whether #isComposite should
> be eliminated.  To my mind, the answer is NO.
>
> I take it as a basic guideline about abstract data type design
> that if an operation may fail, there should be a way for the caller
> to determine *in advance* whether the operation will fail, if that is
> reasonably practical.

"Reasonably practical" could be a whole debate in and of itself, but I would
certainly agree that knowing in advance is desirable.

> For example, since (aCollection last) will
> fail if the collection is empty, we need (aCollection isEmpty).  But
> since (aCollection last) will _also_ fail if aCollection is not a
> sequenceable collection, we need aCollection isSequenceable.  (In fact,
> for the sake of #removeLast, we should also have #isOrdered.)

This ends up with a problem: if we want to get last we certainly don't want
to have to do multiple checks just to find out if we're going to be able to
do it. Though, in this case, (aCollection last) might have a reasonable
behavior for unordered and empty lists and such and thus might not require
the checks.

> How is a caller to know whether it is acceptable to call #addChild:
> without some way of testing whether the Thingy it wants to add to is
> composite or not?  Well, with the design I've presented (where operations
> only _exist_ in classes where they can succeed at least some of the time)
> the test aThingy respondsTo: #addChild: will do the job.  Slower and
> less clearly than #isComposite, but it will do the job.

And I would think (forgive my naivete if it is not so) respondsTo: could be
optimized to use the same/similar code as message dispatch in which case it
would only be slightly slower than #isComposite.

> Or we could
> whack in a rather unappetising
>     addChild: aThingy self error: 'non-composites have no children'
> and catch the exception, which is also slower and less clear.

On a brief digression I'd like to suggest yet another idea. This is
smalltalk after all and can do powerful things such as:

If you #addChild: aThingy to something that isn't a composite, then change
it to a composite and add it. I realize this wouldn't work necessarily in
all cases, but for what I'm taking the above example to be: instead of
making everything a composite, we could make specialized classes to optimize
for single cases, but as soon as someone requires composite functionality,
you morph into it.

However, then you might want some sort of #canAddChild because even if
currently #isComposite is false, #canAddChild could be true.

> Andres Valloud appears to be arguing that when you see a method doing
>     aThingy isComposite
>         ifTrue: [~~a~~]
>         ifFalse: [~~b~~]
> then that means that there should be some method in Thingy which does
> this job.  Of course, we could always just add
>
>     Thingy
>     >>ifComposite: blockC ifSimple: blockS ^blockS value
>     subclass CompositeThingy
>         >>ifComposite: blockC ifSimple: blockS ^blockC value
>
> but via
>     aThingy ifComposite: [true] ifSimple: [false]
> this is equivalent to #isComposite, and is not the _kind_ of method
> that Andres Valloud has in mind.  He has in mind methods with
> intention-revealing names that perform some sensible *specific*
> combination of operations.
>
> Sometimes you can create such methods, and clearly you should.
>
> But sometimes you cannot create such methods without introducing coupling.
>
> Frankly, the idea of filling Object with dozens of methods tightly
> coupled to all sorts of things horrifies me.  Once again, the cure is
> worse than the complaint.

I'm not sure I follow what you are referring to "the cure is worse than the
complaint". Are you specifically referring to the proposed cure by Andres?

> Checking the value of a boolean expression using ifTrue/ifFalse is FAST.
> Much faster than calling a method where you have to pass a real block,
> which you sometimes do if you are to avoid a nasty case of coupling.
>
> Let's take an example from the 3.5 image and see what we can make of it.
> It's #isSequenceable.
>
>     Collection>>
>     adaptToCollection: rcvr andSend: selector
>         "somewhat condensed"
>
>         rcvr isSequenceable & self isSequenceable
>             ifFalse: [self error: '....'].
>         ^rcvr with: self collect: [:re :se |
>             re perform: selector with: se]
>
> There are *two* sends of #isSequenceable here.
> We can eliminate the test of self isSequenceable by putting
>
>     Collection>>
>     adaptToCollection: rcvr andSend: selector
> self error: '...'
>
>     SequenceableCollection>>
>     adaptToCollection: rcvr andSend: selector
>         rcvr isSequenceable ifFalse: [self error: '...'].
> ^rcvr with: self collect: [:re :se |
>     re perform: selector with: se]
>
> and we can eliminate the second by noticing that
> #with:collect: is only defined for SequenceableCollection and its
> descendants, and the function of the test is to produce a better
> error message.  So our final result is
>
>     Collection>>
>     adaptToCollection: rcvr andSend: selector
>         self error: 'arithmetic on non-sequenceable collection'.
>
>     with: anotherCollection collect: aBlock
>         self error: 'only sequenceable collections can be combined this
way'.
>
>     SequenceableCollection>>
>     adaptToCollection: rcvr andSend: selector
>         ^rcvr with: self collect: [:re :se |
>             re perform: selector with: se]
>
> This version of #adaptToCollection:andSend: _is_ clearer and I expect
> it would be faster.
>
> Note that if the two methods in Collection were removed, the only
> effect would be the nature of the error message: reasonably specific
> messages would be replaced by "does not understand".
>
> For this example, Andres Valloud's recommendation seems to be RIGHT.

Sounds good to me, too.

> [snip]

>  I suspect that it should be
> possible to eliminate the uses of #isColor, but NOT the uses of
> #isOpaque and #isTranslucent.

Here's an interesting thing, though. opaque and translucent are not
questions regarding the "capability" of the object, rather they are
referring to properties of the object. In other words, these are NOT
examples of #isFooness. Rather they would in effect map to an instance
variable (whether directly or conceptually) that determines that property of
the color as opposed to what our discussion revolves around. Because of the
same naming convention for these two cases there can be some confusion, but
I think it's important to distinguish them.

>     BorderedMorph>>borderColor: colorOrSymbolOrNil
>         self doesBevels ifFalse: [
>     "Why no error message here?"
>             colorOrSymbolOrNil isColor ifFalse: [^self]].
> borderColor = colorOrSymbolOrNil ifFalse: [
>             "Why no error checking here?"
>     borderColor := colorOrSymbolOrNil.
>     self changed].
>
> Ah.  Now we know that borderColor can be absolutely anything.
>     (BorderedMorph new borderColor: 'whee!')
> gets through with no complaints, leaving the 'borderColor' instance
> variable set to a string.  Is this a good idea?

Umm... wouldn't that be a resounding no? I would think borderColor better be
a color (at the very least in the #isColor sense) no matter what because if
it's not than it surely is not named appropriately.

> It appears that the
> (borderColor isColor) test really is necessary; having
> #isOpaque and #isTranslucent defined on Symbol and UndefinedObject
> doesn't really appeal.  Actually, neither does letting borderColor
> be absolutely anything.  Would a little bit of error checking _really_
> be such a bad idea?  And how would that error checking work, if it did
> not have something very like #isColor to check for?
>
>     BorderedMorph>>borderStyle
>         "Work around the borderWdith/borderColor pair"
>         |style|
>
>         borderColor ifNil: [^BorderStyle default].
> borderWidth isZero ifTrue: [^BorderStyle default].
> style := self valueofProperty: #borderStyle
>               ifAbsent: [BorderStyle default].
>         (borderWidth = style width and: [
>             "Hah! Try understanding this..."
>             borderColor == style style "#raised/#inset etc" or: [
>             #simple == style style and: [
>             borderColor = style color]]]
> ) ifFalse: [
>             style := borderColor isColor
>                 ifTrue:  [BorderStyle width: borderWidth
>                                       color: borderColor]
>                 ifFalse: [(BorderStyle perform: borderColor "argh.")
>                            width: borderWidth].
>     self setProperty: #borderStyle toValue: style].
>         ^style trackColorFrom: self
>
> We learn that the three cases for borderColor are
>     nil     : use the default border style
>     a Symbol: selector of a method in BorderStyle, a border style name
>     a Color : a colour.
> So in fact 'borderColor' is not always a color, and while setting it to
> a string is allowed by #borderColor:, it will give rise to serious
> problems later.  #borderColor: really SHOULD be stricter.

Right. Seems to me they're using that variable to mean multiple things which
I understand to be a "bad thing".

> [snip]

> This really does seem to be a legitimate use of #isColor.
>
> There are lots more uses of #isColor, and I have other things to do today.
>
> I think we have three conclusions from this incomplete study.
>
> (1) Morphic is a mess.  THREE CHEERS AND THREE CHEERS MORE
>     for the Morphic Cleaning Project.
>
> (2) Andres Valloud is absolutely right that uses of #isFoo *can*
>     be a signal that some of the code is rotten and needs refactoring;
>     I have to agree that it is always appropriate to at least
>     *consider* whether the kind of alternative he suggests would be
>     more appropriate, and it very often is.
>
> (3) However, there are cases (like Color>>#= and Morph>>color:)
>     where it is difficult to conceive of any replacement which would
>     not be worse.

Excellent. I think I'll do my own summary as well just to gear back and have
something to attack if there is any remaining attacking to do. Do you also
agree that #isColor although apparently useful shouldn't be defined on
Object, but rather should be on an appropriate place in the inheritance
hierarchy (my initial uneducated guess would be on Color itself)?

-joel