CODEWITHSUNDEEP
smalltalkpharo
CodexArcanum
CodexArcanum

Reputation: 4016

What is the best way to enforce certain values on message arguments in smalltalk?

I'm working on a simple board game in Pharo, and I've got a method on my Board that adds objects to a cell. Cells are simply a dictionary of Points on Objects.

As part of the method, I wanted to enforce that a Point should be greater than zero, but less than the width and height of the board, in other words, it should actually be on the board. What is the best way to do this?

My current attempt looks like this:

at: aPoint put: aCell

((((aPoint x > self numberOfRows) 
    or: [aPoint x <= 0]) 
    or: [aPoint y > self numberOfColumns ]) 
    or: [aPoint y <= 0]) 
    ifTrue: [ self error:'The point must be inside the grid.' ].

self cells at: aPoint put: aCell .

Kind of lisp-y with all those parens! But I can't use the short-circuiting or: without closing off each expression so it evaluates as a boolean and not a block (or as the or:or:or:or: message). I could use the binary operator | instead and for-go short circuiting, but that doesn't seem right.

So what is the properly Smalltalk-ish way to handle this?

Upvotes: 3

Views: 241

Answers (3)

Randal Schwartz
Randal Schwartz

Reputation: 44056

Any time things are that heavily nested, it's time to call another method.

isValidPoint: aPoint
  aPoint x > self numberOfRows ifTrue: [^ false].
  aPoint x <= 0 ifTrue: [^ false].
  aPoint y > self numberOfColumns ifTrue: [^ false].
  aPoint y <= 0 ifTrue: [^ false].
  ^ true.

In general, your methods should be relatively flat. If not, time to refactor.

Upvotes: 4

Igor Stasenko
Igor Stasenko

Reputation: 1137

You can simply prefill the 'cells' dictionary with all points which is valid within the range, i.e. somewhere in initialization you put:

1 to: numberOfRows do: [:y |
  1 to: numberOfCols do: [:x |
     cells at: x@y put: dummy "or nil " ] ]

then your method for adding a cell at given point will look as simple as:

at: aPoint put: aCell

   self cells at: aPoint ifAbsent: [ self error: 'The point must be inside the grid.' ].
   self cells at: aPoint put: aCell .

There's also a helper method #between:and: , which you can use to minimize the code clutter:

((aPoint x between: 1 and: self numCols) and: [
 aPoint y between: 1 and: self numRows ]) ifFalse: [ ... bummer ... ]

Upvotes: 3

Lukas Renggli
Lukas Renggli

Reputation: 8947

Typically the or: are nested like this:

(aPoint x > self numberOfRows 
    or: [ aPoint x <= 0  
    or: [ aPoint y > self numberOfColumns
    or: [ aPoint y <= 0 ] ] ])
        ifTrue: [ self error: 'The point must be inside the grid.' ].

Your nesting is short-circuting but less efficient because of repeated tests of the first argument (check the bytecode to see the difference).

Alternative you can use assert: or assert:description: that is defined on Object:

self
    assert: (aPoint x > self numberOfRows 
        or: [ aPoint x <= 0  
        or: [ aPoint y > self numberOfColumns
        or: [ aPoint y <= 0 ] ] ])
    description: 'The point must be inside the grid.'

Upvotes: 6

Related Questions