CODEWITHSUNDEEP
smalltalksqueak
Ariel Pinchover
Ariel Pinchover

Reputation: 654

Refactoring a method in smalltalk

I'm a new user (actually studying it in a course) in Smalltalk (Squeak). I have a method to check if one rectangle equals to a given rectangle which looks like this:

isEqual:givenRec
    self a = givenRec a
    ifTrue: [
        self b = givenRec b
        ifTrue: [
            ^true
        ].
        ^false
    ].
    self b = givenRec a
    ifTrue: [
        self a = givenRec b
        ifTrue: [
            ^true
        ].
        ^false
    ].
    ^false

my question is - is there a way to better write this? for it to be more compact?

also - why cant I refer to a which is instanceVariableNames with self inside methods? thanks for the help!

EDIT:

this is how the class is defined:

MyShape subclass: #MyTriangle
    instanceVariableNames: 'a b c'
    classVariableNames: ''
    poolDictionaries: ''
    category: 'Ex2'

MyShape simply derives from Object and has nothing.

Upvotes: 3

Views: 267

Answers (3)

Leandro Caniglia
Leandro Caniglia

Reputation: 14858

I would have written the method as

equals: aTriangle
  a = aTriangle a ifFalse: [^false].
  b = aTriangle b ifFalse: [^false].
  ^c = aTriangle c

Note by the way that I'm using the selector #equals: rather than #isEqual: because the latter doesn't read so well in English (it would if it had been #isEqualTo:, but #equals: is shorter).

Another remark I would add here is that you could have redefined #= instead of adding a new comparison selector. In that case, however, you have to pay attention to a couple of additional things:

= aTriangle
  self class = aTriangle class ifFalse: [^false].
  a = aTriangle a ifFalse: [^false].
  b = aTriangle b ifFalse: [^false].
  ^c = aTriangle c

The reason for the class check is to make sure that #= is robust, i.e., it doesn't signal a MessageNotUnderstood exception. The other thing is that every time you (re)define #= you should also (re)define #hash in such a way that t1 hash = t2 hash whenever t1 = t2. For example

hash
  ^(a hash + b hash + c hash) hash

Note that this proposal for #hash is not the best one, but this is not the place to discuss the problem of writing good #hash functions.

UPDATE

Here is my version when the order doesn't matter

equals: aTriangle
  | sides |
  self = aTriangle ifTrue: [^true].
  sides := Set with: a with: b with: c.
  (sides includes: aTriangle a) ifFalse: [^false].
  (sides includes: aTriangle b) ifFalse: [^false].
  ^(sides includes: aTriangle c)

The first comparison is to speed up the method avoiding the Set when the order is the same.

Upvotes: 0

Stephan Eggermont
Stephan Eggermont

Reputation: 15907

You should probably not write it like this, but there are situations where meta programming this is useful 

isEqual: givenRec
  #(a b c) do: [ :selector |
    (self perform: selector) = (givenRec perform: selector) ifFalse: [
      ^false]].
  ^true

Upvotes: 0

Tobias
Tobias

Reputation: 3110

You can make that more compact, yes.

First, besides #ifTrue: there is also #ifFalse: and #ifTrue:ifFalse:, roughly having the effect of if-not--then and if--then--else.

But also, we already have a logical AND condition, so why not use that:

isEqual: givenRec

    (self a = givenRec a and: [self b = givenRec b])
        ifTrue: [^ true].
    (self b = givenRec a and: [self a = givenRec b])
        ifTrue: [^ true].
    ^false

With #ifTrue:ifFalse:

isEqual: givenRec

    (self a = givenRec a and: [self b = givenRec b])
        ifTrue: [^ true]
        ifFalse: [^ (self b = givenRec a and: [self a = givenRec b])]

Also, we can do the return around the whole statement:

isEqual: givenRec

    ^ (self a = givenRec a and: [self b = givenRec b])
        ifTrue: [true]
        ifFalse: [self b = givenRec a and: [self a = givenRec b]]

But ifTrue: [true] is a bit redundant, let's use #or:

isEqual: givenRec

    ^ (self a = givenRec a and: [self b = givenRec b]) or: 
      [self b = givenRec a and: [self a = givenRec b]]

Nice and sweet, we also see the logical structure pretty easily. (note that I diverge from common formatting styles to point out the similarities and differences in the two logical expressions).

We now have only one return ^, no #ifTrue:…

For the instance variable question:

When you define some instance variables in the class like you did, you can use them literally in your code to access them:

Object subclass: #Contoso
    instanceVariableNames: 'things'
    classVariableNames: ''
    poolDictionaries: ''
    category: 'Examples'

isThingPlusThreeSameAs: anObject

    ^ thing + 3 = anObject

But typically, it is better to refer to instance variables via getters and setters, commonly called accessors. You have to write them manually or use the 'create inst var accessor' menu item of the second context menu (via “more…”) of a class in the browser:

This will generate accessor methods of the form

thing

    ^ thing

thing: anObject

    thing := anObject

and you can use them in other methods like this

isThingPlusThreeSameAs: anObject

    ^ self thing + 3 = anObject

Upvotes: 4

Related Questions