3

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?

1
  • Remember you can automagically "format" for code, removing all the extra parentheses via Cmd+r or Ctrl+r or right clicking on the method body and choosing format.
    – unom
    Feb 18, 2015 at 8:02

3 Answers 3

6

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.'
1
  • Ah that's perfect. Thanks for the assert syntax as well, I was thinking that assert might be useful. Dec 11, 2010 at 19:09
4

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.

1
  • That's excellent advice, that I did indeed end up taking. Especially when a later method requires two valid points and I needed to check both of them! Dec 13, 2010 at 21:17
3

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 ... ]
1
  • I'd forgotten about between:and: that's a much cleaner way to express things. Thanks! Dec 11, 2010 at 19:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.