viernes, 3 de julio de 2009

Respuesta: ¿Cómo mejorar esta clase?

El otro día postee una pregunta sobre cómo mejorar la clase MouseOverHandler de Squeak/Pharo... no hubo ningún comentario lo que creo que significa que nadie se animó a hacerlo! (o claro, nadie leyó el post...). En fin, creo que nadie se animó a hacerlo porque realmente no es fácil y sólo ver el código desalienta a cualquiera, pero bueno, yo lo hice hace una para de semanas atrás así que les comento cómo.
Primero, ¿cuál es el problema con esta implementación? El principal es que el método #processMouseOver: es muy difícil de entender. Es muy largo y complicado, así que hay que empezar por tratar de entender que hace y mejorarlo. Para ello vamos a utilizar el refactoring de extract method para reemplazar colaboraciones por mensajes que revelen su intención. Lo interesante es que el método está comentado justamente en las secciones que vamos a extraer, lo cual muestra como a veces en vez de comentar código simplemente hay que escribirlo con intention revealing. Veamos como queda.
Reemplacemos este código:

hand := anEvent hand.
leftMorphs := mouseOverMorphs asIdentitySet.
"Assume some coherence for the number of objects in over list"
overMorphs := WriteStream on: (Array new: leftMorphs size).
enteredMorphs := WriteStream on: #().

Por:
self initializeProcessMouseOver.

Luego reemplacemos:
"Now go looking for eventual mouse overs"
hand handleEvent: anEvent asMouseOver.
"Get out early if there's no change"
(leftMorphs isEmpty and: [enteredMorphs position = 0])
ifTrue: [^leftMorphs := enteredMorphs := overMorphs := nil].
focus := hand mouseFocus.

Por:
self handleAsMouseOver: anEvent.

También reemplacemos: (no se preocupen en leer todo el código, simplemente tomen un dimensión de qué estamos reemplazando y por quién)
"Send #mouseLeave as appropriate"
evt := anEvent asMouseLeave.
"Keep the order of the left morphs by recreating it from the mouseOverMorphs"
leftMorphs size > 1
ifTrue: [leftMorphs := mouseOverMorphs select: [:m | leftMorphs includes: m]].
leftMorphs do:
[:m |
(m == focus or: [m hasOwner: focus])
ifTrue:
[localEvt := evt transformedBy: (m transformedFrom: hand).
m handleEvent: localEvt]
ifFalse: [overMorphs nextPut: m]].

Por:
self handleAsMouseLeave: anEvent.

También reemplacemos:
"Send #mouseEnter as appropriate"
evt := anEvent asMouseEnter.
enteredMorphs ifNil:
["inform: was called in handleEvent:"

^leftMorphs := enteredMorphs := overMorphs := nil].
enteredMorphs := enteredMorphs contents.
enteredMorphs reverseDo:
[:m |
(m == focus or: [m hasOwner: focus])
ifTrue:
[localEvt := evt transformedBy: (m transformedFrom: hand).
m handleEvent: localEvt]].

Por:
self handleAsMouseEnter: anEvent.

Y por último:
"And remember the over list"
overMorphs ifNil:
["inform: was called in handleEvent:"

^leftMorphs := enteredMorphs := overMorphs := nil].
mouseOverMorphs := overMorphs contents.
leftMorphs := enteredMorphs := overMorphs := nil

Por:
self rememberOverList

Bueno, no se que les parece (o si quedó claro como lo escribí), pero simplemente hay que agarrar cada conjunto de colaboraciones que tiene un comentario sobre que hacen y extraerlas a otro método. En conclusión con algunos cambios más el método #processMouseOver: quedá así:

processMouseOver: anEvent

self initializeProcessMouseOver.
self handleAsMouseOver: anEvent.
self hasLeftMorphsChanged ifTrue: [
self handleAsMouseLeave: anEvent.
self handleAsMouseEnter: anEvent.
self rememberOverList ].

self initializeTrackedMorphs

¿Se entiende mejor qué hace no?. Fijensé como un cambio tan sencillo, simplemente usando extract method, mejora el diseño considerablemente. Por supuesto que hay que seguir utilizando esta técnica para cada uno de los nuevos métodos porque siguen siendo muy complejos y al hacerlo se puede ver que hay código repetido, de hecho el mismo refactoring browser al realizar la extracción lo sugiere. No voy a mostrar todo este proceso porque es similar al anterior y no quiero hacer el post muy pesado (más de lo que es!).
Por último, también realicé otros cambios como por ejemplo no usar nil para inicializar las variables de instancia que utiliza para realizar el procesamiento, sino inicializarlas con colecciones vacías de tal manera que nunca se produzca un MNU.
En conclusión, el código quedó así:

Object subclass: #MouseOverHandler
instanceVariableNames: 'mouseOverMorphs enteredMorphs overMorphs leftMorphs'
classVariableNames: ''
poolDictionaries: ''
category: 'Morphic-Events'!
-------------------------------------------------------------------------------
processMouseOver: anEvent

self initializeProcessMouseOver.
self handleAsMouseOver: anEvent.
self hasLeftMorphsChanged ifTrue: [
self handleAsMouseLeave: anEvent.
self handleAsMouseEnter: anEvent.
self rememberOverList ].

self initializeTrackedMorphs
-------------------------------------------------------------------------------
initializeProcessMouseOver

leftMorphs := mouseOverMorphs asIdentitySet.
overMorphs := WriteStream on: (Array new: leftMorphs size).
enteredMorphs := WriteStream on: #()! !
-------------------------------------------------------------------------------
handleAsMouseEnter: anEvent

| asMouseEnterEvent |

asMouseEnterEvent := anEvent asMouseEnter.
enteredMorphs := enteredMorphs contents.
enteredMorphs reverseDo: [ :anEnteredMorph |
self inform: asMouseEnterEvent to: anEnteredMorph originatedFrom: anEvent ifNotFocusedDo: [] ]
-------------------------------------------------------------------------------
handleAsMouseLeave: anEvent

self keepLeftMorphsOrder.
self informMouseLeaveToLeftMorphsUsing: anEvent
-------------------------------------------------------------------------------
handleAsMouseOver: anEvent

anEvent hand handleEvent: anEvent asMouseOver.
-------------------------------------------------------------------------------
noticeMouseOver: aMorph event: anEvent
"Remember that the mouse is currently over some morph"

leftMorphs remove: aMorph ifAbsent: [ enteredMorphs nextPut: aMorph ].
overMorphs nextPut: aMorph.
-------------------------------------------------------------------------------
initialize
mouseOverMorphs := #().

self initializeTrackedMorphs
-------------------------------------------------------------------------------
initializeTrackedMorphs

leftMorphs := OrderedCollection new.
overMorphs := WriteStream on: #().
enteredMorphs := WriteStream on: #().
-------------------------------------------------------------------------------
hasLeftMorphsChanged

^(leftMorphs isEmpty and: [ enteredMorphs position = 0 ]) not
-------------------------------------------------------------------------------
inform: evt to: aLeftMorph originatedFrom: anEvent ifNotFocusedDo: aBlock

^ (self is: anEvent withFocusOver: aLeftMorph)
ifTrue: [ self transform: evt from: anEvent andSendTo: aLeftMorph ]
ifFalse: aBlock
-------------------------------------------------------------------------------
informMouseLeaveToLeftMorphsUsing: anEvent

| asMouseLeaveEvent |

asMouseLeaveEvent := anEvent asMouseLeave.

leftMorphs do: [ :aLeftMorph |
self inform: asMouseLeaveEvent to: aLeftMorph originatedFrom: anEvent ifNotFocusedDo: [ overMorphs nextPut: aLeftMorph ] ]
-------------------------------------------------------------------------------
is: anEvent withFocusOver: aMorph

| focusedMorph |

focusedMorph := anEvent hand mouseFocus.
^ aMorph = focusedMorph or: [ aMorph hasOwner: focusedMorph ]
-------------------------------------------------------------------------------
keepLeftMorphsOrder

leftMorphs size > 1 ifTrue: [ leftMorphs := mouseOverMorphs intersection: leftMorphs ]
-------------------------------------------------------------------------------
rememberOverList

mouseOverMorphs := overMorphs contents.
-------------------------------------------------------------------------------
transform: anEvent from: originalEvent andSendTo: aMorph

| transformedEvent |

transformedEvent := anEvent transformedBy: (aMorph transformedFrom: originalEvent hand).
^ aMorph handleEvent: transformedEvent
-------------------------------------------------------------------------------
¿Mucho más claro no?

3 comentarios:

pancho dijo...

buen ejemplo, pero mi jefe preguntaria: solucionaste el problema? (el MNU #includes:)

Hernan Wilkinson dijo...

Muy buena pregunta!
La realidad es que la solución a ese error es muy compleja para lo que quería mostrar en el post, así que no, ese código no lo soluciona pero por lo menos no hace que se te cuelge pharo cuando antes se producía...

Anónimo dijo...

Ist Einverstanden, es ist das bemerkenswerte StГјck viagra bestellen cialis kaufen ?sterreich [url=http//t7-isis.org]cialis online[/url]