Discussion, demonstrating error handling

[bencoman]

Tim & Sam, Seeking your opinions. Pharo has a powerful and flexible exception handling system that I don't feel we are demonstrating. As I've been working through the Bowling exercise, I've developed some concerns with this test pattern...

test15_RollsCannotScoreNegativePoints
	| result |
	self
		should: [ result := bowlingCalculator scoreRolling: -1 after: #() ]
		raise: BowlingError
		whoseDescriptionIncludes: 'Negative roll is invalid'
		description: 'Should get an error with the correct description'

because:

1. It doesn't teach students how errors to handle errors in application code.
   I feel the following teaches students more...

test15_RollsCannotScoreNegativePoints
	| testError |
	[ bowlingCalculator scoreRolling: -1 after: #() ] 
		on: Error do: [ :error | testError := error ].
	self assert: testError notNil.	
	self assert: testError description equals: 'Negative roll is invalid'.  

2. Its using a facility not in the base image, so students can be confused when they try to copy the pattern in their own projects. [EDIT: Whoops, my presumption was wrong. It is in the base image]
3. The Error-instance is not available for inspection.
4. With my Bowling solution I want to demonstrate the use of custom errors by seeding the following with the downloaded exercise...

Error subclass: #BowlingError
	instanceVariableNames: 'artifact description'
	classVariableNames: ''
	package: 'Exercism-Bowling'

BowlingError>>artifact: anObject
	artifact := anObject 

BowlingError>>artifact
	^ artifact

BowlingError>>description: errorString
	description := errorString

BowlingError>>description
	^ 'Bowling error: ' , description

Frame>>raiseBowlingError: errorString artifact: anObject
	<exercismHint>
	"Raises a custom error with state holding the troublesome entity"
	(BowlingError new artifact: anObject; description: errorString) signal

FrameTest>>test16_customError
	| error |
	[ Frame new raiseBowlingError: 'My custom error' artifact: 42] 
		on: BowlingError do: [ :err | error := err ].
	self assert: error notNil.	
	self assert: (error description includesSubstring: 'My custom error').
	self assert: error artifact equals: 42.

FrameTest>>test17_negativeRollError
	| error |
	[ Frame new roll: -1 ] 
		on: BowlingError do: [ :err | error := err ].
	self assert: error notNil.	
	self assert: (error description includesSubstring: 'Negative roll is invalid').  
	self assert: error artifact equals: -1

and leave students to write...

Frame>>roll: anInteger 
	anInteger < 0 ifTrue: [ self raiseBowlingError: 'Negative roll is invalid' artifact: anInteger ].
	self isFull ifTrue: [ ^ self nextFrame roll: anInteger ].
	frameRolls add: anInteger

[macta]

Hi Ben - you do find interesting things...

The current test pattern is of course auto-generated and generic - but better than the previous auto-generated one.

I see what you are getting at - and think you are on to something, but i'm not so keen on the 3 post asserts (the first one checks that an exception even happened).

I think we definitely could up the anti on being specific about students creating a specific exception (it was fine for an early exercise to use anything handy - but probably before bowling we should have an exercise specify an exercise specific error - so raise: MyError whoseDescr....:

Back to the second part - I think the use of higher order assertions is good (and hence my aversion to 3 asserts and a variable) - however I think you are correct that upping the anti on using a smarter exception is a good thing - so why don't we provide:

self should: [ result := bowlingCalculator scoreRolling: -1 after: #() ]
      raise: BowlingError
       specifying: [:ex |
                     self assert: ex message includes: 'my message'.
                     self assert: ex artefect equals: 42 ]

This is the neater way (note it also raises the issue that SUnit is backwards with beginsWith and includes support - raised recently when some collection extensions were added and fought for as CI was failing with undeciperhable messages). So I would add the beingsWith and Includes support so students get a clean message, and maybe we can get them added to SUnit. We could add stateSpec which has a nice dsl for all this, but it adds complexity.

[bencoman]

I like that. It covers points (3) & (4).
Actually just discovered there is already should:raise:withExceptionDo:.
Point (2) was incorrect.
We need to do (1) at some point, but I'm happy for your pushback its not for every exercise.

[samWson]

I've been reading up on exceptions just last week. The book Deep into Pharo (Language -> Handling Exceptions) covers exceptions very well. The style you've shown @bencoman very much fits with what I read and I like your reasoning. I'm happy to go along with the conclusion you two have already come to.