Defensive programming or not

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Defensive programming or not

Kasper Osterbye
Cheers all,

In trying to fix one of the issues in Pharo, I came to wonder about the prefered style in in dealing with preconditions to methods in Pharo.

In the method Random>>nextInt: anInteger
  • There is a check if anInteger is negative
  • There is no check that anInteger is anInteger
When should I put a guard there, and throw an early warning, and when is it part of the (often implicit pre-condition)?

Best,

Kasper
Reply | Threaded
Open this post in threaded view
|

Re: Defensive programming or not

Sven Van Caekenberghe-2
Hi Kasper,

> On 23 Sep 2019, at 10:18, Kasper Osterbye <[hidden email]> wrote:
>
> Cheers all,
>
> In trying to fix one of the issues in Pharo, I came to wonder about the prefered style in in dealing with preconditions to methods in Pharo.
>
> In the method Random>>nextInt: anInteger
> • There is a check if anInteger is negative
> • There is no check that anInteger is anInteger
> When should I put a guard there, and throw an early warning, and when is it part of the (often implicit pre-condition)?
>
> Best,
>
> Kasper

This is very good question.

In my opinion, in a dynamically typed language you should not explicitly enforce all types.

It is OK to rely on MessageNotUnderstood exceptions, it is the ultimate protection.

Of course, you could help your users/callers by throwing more useful exceptions, when that makes sense.

There is also a cost with checking (too often).

Furthermore, #isInteger or #isString are close testing the for a class, which is not considered good object design.

In the case of Random>>nextInt: speed seems important, but a robust interface with type checking has some value too. Hard to decide.

Sven



Reply | Threaded
Open this post in threaded view
|

Re: Defensive programming or not

Richard O'Keefe
In reply to this post by Kasper Osterbye
This is perhaps the biggest question I've faced working on my own Smalltalk,
and it is connected to an annoyance.

My answer is that when I am *using* library code, I want my mistakes
to be detected
at "interface" level.  If, for example, I do
((1 to: 5) asOrderedCollection) at: 2.5 put: #fred; yourself
I want the mistake to be detected and I want it to be reported as coming from
OrderedCollection>>at:put:
To my shock and dismay, Pharo just answered an OrderedCollection(1 #fred 3 4 5).
VisualWorks, VisualAge, ST/X, Dolphin, and GNU ST all raise an exception if the
index is not an integer.

Can anyone explain to me why
            ^ index isNumber
                ifTrue: [ self at: index asInteger put: value]
                ifFalse: [ self errorNonIntegerIndex] ].
is thought to be an improvement over
            ^  self errorNonIntegerIndex ].
when it comes to Object>>[basic]at:[put:]?  (The name
#errorNonIntegerIndex suggests
that other numbers were not originally allowed.)

One thing I expect from an array-like object is that if i and j are
different and x and y
are different, then  anArray at: i put: x; at: j put: y; at: i
should answer x.  But
   (Array new: 1) at: 1.1 put: #tin; at: 1.2 put: #pan; at: 1.1
is #pan, not #tin.

To return to my initial point.  Assuming a version of Smalltalk in
which sequence indices
*are* checked, I want (anOrderedCollection at: 2.5) to report the
error as coming from
*this* message, not some internal message sent to some internal object
I have no normal
access to.

The annoyance is that this matters less than it should because while
the exception
*mechanism* is defined by the ANSI Smalltalk standard, almost no
*exceptions* were
defined, so there are in practical terms no standard exceptions to
raise or handle.

What I did for my own Smalltalk was
(1) notice that a check (index between: 1 and: size) let Fractions,
ScaledDecimals,
     FloatE, FloatD, FloatQ, DualNumbers, and QuadraticSurds slip through.
(2) face-palm "how stupid of me! of COURSE that's not right"
(3) introduce a new fast primitive (index integer_between: 1 and: size)
     where x integer_between: y and: z turns into
     (((x)|(y)|(z))&TAG) == 0 && (SignedWord)(y) <= (SignedWord)(x) &&
     (SignedWord)(x) <= (SignedWord)(z)
     which I suppose should really be called something like #basicBetween:and:.
(4) Discover that there were far more places where I should have been using
     integer_between:and: than I had originally expected.  It's about
a 50-50 split.

My point here is that if it seems as though it might be too expensive to check,
perhaps you have the wrong tools for checking.  Or perhaps it is possible to
weaken your precondition so that you don't need to report an errror.

As an example of that, I ran into a problem in Squeak where
  collection1 removeAll: collection2
went crazy if collection1 and collection2 were the same.  My library checks
for that and makes it work.

We sometimes say "Use the source, Luke!"  But that's no good if Luke needs
to know something that isn't *in* the source.  So if a method has a precondition
that is not a consequence of the class invariant and is not checked, it really
needs to be commented.

On Mon, 23 Sep 2019 at 20:19, Kasper Osterbye <[hidden email]> wrote:

>
> Cheers all,
>
> In trying to fix one of the issues in Pharo, I came to wonder about the prefered style in in dealing with preconditions to methods in Pharo.
>
> In the method Random>>nextInt: anInteger
>
> There is a check if anInteger is negative
> There is no check that anInteger is anInteger
>
> When should I put a guard there, and throw an early warning, and when is it part of the (often implicit pre-condition)?
>
> Best,
>
> Kasper