Is This Overkill?

3 May 2007 3:52 PM

Mike Hofer

I've been maintaining some source code, and I've noticed some radical
inconsistency in the code, particularly in the way that parameters are
validated. I've noticed that this seems to have been pretty much the
case throughout my career.

For instance, consider the following block:

If siteId <= 0 Then
    Throw New ArgumentOutOfRangeException("siteId")
End If
If name Is Nothing Then
    Throw New ArgumentNullException("name")
End If
If name = String.Empty Then
    Throw New ArgumentException("Empty strings are not permitted.",
"name")
End If
If buildingIdToIgnore < -1 Then
    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
End If

Sometimes, this code is collapsed as follows:

      If siteId <= 0 Then Throw New
ArgumentOutOfRangeException("siteId")
      If name Is Nothing Then Throw New ArgumentNullException("name")
      If name = String.Empty Then Throw New ArgumentException("Empty
strings are not permitted.", "name")
      If buildingIdToIgnore < -1 Then Throw New
ArgumentOutOfRangeException("buildingIdToIgnore")

Sometimes, it's omitted altogether. Somtimes, not all of the
parameters to the procedure are validated. And in many cases, the
messages aren't consistent from one parameter validation to the next.
(Resource files would go a long way towards solving that, but that's a
whole different thread.)

What I've been pondering for some time is a way to make it easier to
validate method parameters. My *theory* is that it's a lot of gangly
code to write If...Then...Throw...End If, which leads to short-cuts
and, eventually, outright omissions. So, a way to abbreviate that and
make the code clearer might help to encourage consistent use of
parameter validation, which should (in theory) improve code quality
throughout the system.

So I have this model I'm working on, and before I go completely off
the deep end with it, I thought I'd run it by you to see what you
thought of it.

Essentially, it's based on NUnit asserts to validate parameters
consistently and clearly. So, the code above is changed to this:

Validate.That(siteId, "siteId").IsNotNegative()
Validate.That(name, "name").IsNotNullOrEmpty()
Validate.That(buildingIdToIgnore,
"buildingIdToIgnore").IsGreaterThan(-2)
Validate.That(connection).IsOpenAndIsNotNull()

My tests for the model are going very well. I think that once I get it
going, it could provide a very cool, extensible validation framework
for parameters. The "framework" is in a separate class library, and
the messages are in a string resource file, providing consistency.

The big caveat, of course, is that there's no guarantee that it solves
the *enforcement* problem. There's also the problem that FxCop thinks
I'm not validating parameters when I am.

So the question I have is, is this even worth doing? When an existing
code base is refactored to use it, it looks much cleaner, and appears
to be easier to read and maintain. But the question remains: is this a
real problem, or am I addressing an issue that doesn't really exist?

Your input is greatly appreciated.

Thanks!

3 May 2007 4:22 PM

PJ6

I will be very interested to see others' responses to this, but here's my
$.02.

While it is not always practicable or appropriate to do so, tasks of this
nature are best managed with a database.

Yes, I'm leaving a mountain unsaid with this statement, most notably
answering the question "how", but I would recommend this idea and
aspect-oriented programming to you for further study.

Paul

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
> I've been maintaining some source code, and I've noticed some radical
> inconsistency in the code, particularly in the way that parameters are
> validated. I've noticed that this seems to have been pretty much the
> case throughout my career.
>
> For instance, consider the following block:
>
> If siteId <= 0 Then
>    Throw New ArgumentOutOfRangeException("siteId")
> End If
> If name Is Nothing Then
>    Throw New ArgumentNullException("name")
> End If
> If name = String.Empty Then
>    Throw New ArgumentException("Empty strings are not permitted.",
> "name")
> End If
> If buildingIdToIgnore < -1 Then
>    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> End If
>
> Sometimes, this code is collapsed as follows:
>
>      If siteId <= 0 Then Throw New
> ArgumentOutOfRangeException("siteId")
>      If name Is Nothing Then Throw New ArgumentNullException("name")
>      If name = String.Empty Then Throw New ArgumentException("Empty
> strings are not permitted.", "name")
>      If buildingIdToIgnore < -1 Then Throw New
> ArgumentOutOfRangeException("buildingIdToIgnore")
>
> Sometimes, it's omitted altogether. Somtimes, not all of the
> parameters to the procedure are validated. And in many cases, the
> messages aren't consistent from one parameter validation to the next.
> (Resource files would go a long way towards solving that, but that's a
> whole different thread.)
>
> What I've been pondering for some time is a way to make it easier to
> validate method parameters. My *theory* is that it's a lot of gangly
> code to write If...Then...Throw...End If, which leads to short-cuts
> and, eventually, outright omissions. So, a way to abbreviate that and
> make the code clearer might help to encourage consistent use of
> parameter validation, which should (in theory) improve code quality
> throughout the system.
>
> So I have this model I'm working on, and before I go completely off
> the deep end with it, I thought I'd run it by you to see what you
> thought of it.
>
> Essentially, it's based on NUnit asserts to validate parameters
> consistently and clearly. So, the code above is changed to this:
>
> Validate.That(siteId, "siteId").IsNotNegative()
> Validate.That(name, "name").IsNotNullOrEmpty()
> Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
> Validate.That(connection).IsOpenAndIsNotNull()
>
> My tests for the model are going very well. I think that once I get it
> going, it could provide a very cool, extensible validation framework
> for parameters. The "framework" is in a separate class library, and
> the messages are in a string resource file, providing consistency.
>
> The big caveat, of course, is that there's no guarantee that it solves
> the *enforcement* problem. There's also the problem that FxCop thinks
> I'm not validating parameters when I am.
>
> So the question I have is, is this even worth doing? When an existing
> code base is refactored to use it, it looks much cleaner, and appears
> to be easier to read and maintain. But the question remains: is this a
> real problem, or am I addressing an issue that doesn't really exist?
>
> Your input is greatly appreciated.
>
> Thanks!
>

3 May 2007 4:24 PM

Ray Cassick

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
> I've been maintaining some source code, and I've noticed some radical
> inconsistency in the code, particularly in the way that parameters are
> validated. I've noticed that this seems to have been pretty much the
> case throughout my career.
>
> For instance, consider the following block:
>
> If siteId <= 0 Then
>    Throw New ArgumentOutOfRangeException("siteId")
> End If
> If name Is Nothing Then
>    Throw New ArgumentNullException("name")
> End If
> If name = String.Empty Then
>    Throw New ArgumentException("Empty strings are not permitted.",
> "name")
> End If
> If buildingIdToIgnore < -1 Then
>    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> End If
>
> Sometimes, this code is collapsed as follows:
>
>      If siteId <= 0 Then Throw New
> ArgumentOutOfRangeException("siteId")
>      If name Is Nothing Then Throw New ArgumentNullException("name")
>      If name = String.Empty Then Throw New ArgumentException("Empty
> strings are not permitted.", "name")
>      If buildingIdToIgnore < -1 Then Throw New
> ArgumentOutOfRangeException("buildingIdToIgnore")
>
> Sometimes, it's omitted altogether. Somtimes, not all of the
> parameters to the procedure are validated. And in many cases, the
> messages aren't consistent from one parameter validation to the next.
> (Resource files would go a long way towards solving that, but that's a
> whole different thread.)
>
> What I've been pondering for some time is a way to make it easier to
> validate method parameters. My *theory* is that it's a lot of gangly
> code to write If...Then...Throw...End If, which leads to short-cuts
> and, eventually, outright omissions. So, a way to abbreviate that and
> make the code clearer might help to encourage consistent use of
> parameter validation, which should (in theory) improve code quality
> throughout the system.
>
> So I have this model I'm working on, and before I go completely off
> the deep end with it, I thought I'd run it by you to see what you
> thought of it.
>
> Essentially, it's based on NUnit asserts to validate parameters
> consistently and clearly. So, the code above is changed to this:
>
> Validate.That(siteId, "siteId").IsNotNegative()
> Validate.That(name, "name").IsNotNullOrEmpty()
> Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
> Validate.That(connection).IsOpenAndIsNotNull()
>
> My tests for the model are going very well. I think that once I get it
> going, it could provide a very cool, extensible validation framework
> for parameters. The "framework" is in a separate class library, and
> the messages are in a string resource file, providing consistency.
>
> The big caveat, of course, is that there's no guarantee that it solves
> the *enforcement* problem. There's also the problem that FxCop thinks
> I'm not validating parameters when I am.
>
> So the question I have is, is this even worth doing? When an existing
> code base is refactored to use it, it looks much cleaner, and appears
> to be easier to read and maintain. But the question remains: is this a
> real problem, or am I addressing an issue that doesn't really exist?
>
> Your input is greatly appreciated.
>
> Thanks!
>

I like the idea and have thought about doing it myself at some point, just
never got around to it.

I might take it a bit further and somehow move the validation to an engine
scheme type of approach where rules for validation could be stored perhaps
in an XML file and then parameters would be passed in along with their
function names. This would give you a central location to store rules about
what is valid and what is not as well as the ability to perhaps alter
validation schemes dynamically without altering code if needed in the
future.

It may also, depending how it was architected, allow you to better control
rules for values that may be dependent upon other values that have been
passed into the function without having to create a bunch of nested
statements in the code body itself.

No matter what you do though, enforcement of its use is always going to be
an issue. Unless you stand over the shoulder or developers as they code, the
only real way to catch non-compliance would be code reviews.

3 May 2007 6:59 PM

Jon Skeet [C# MVP]

Mike Hofer <kchighl***@gmail.com> wrote:

> I've been maintaining some source code, and I've noticed some radical
> inconsistency in the code, particularly in the way that parameters are
> validated. I've noticed that this seems to have been pretty much the
> case throughout my career.

<snip>

You might want to look at Spec# for inspiration.
http://research.microsoft.com/specsharp/

If you don't mind the stack trace having one more line than is terribly
helpful, you can have a lot of useful exception-throwing validation
methods, e.g.

ValidateNonNull (name, "name");
ValidateRange (siteId > 0, "siteId");

etc

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

3 May 2007 9:10 PM

Mike Hofer

On May 3, 2:59 pm, Jon Skeet [C# MVP] <s***@pobox.com> wrote:
Show quote

> Mike Hofer <kchighl***@gmail.com> wrote:
> > I've been maintaining some source code, and I've noticed some radical
> > inconsistency in the code, particularly in the way that parameters are
> > validated. I've noticed that this seems to have been pretty much the
> > case throughout my career.
>
> <snip>
>
> You might want to look at Spec# for inspiration.http://research.microsoft.com/specsharp/
>
> If you don't mind the stack trace having one more line than is terribly
> helpful, you can have a lot of useful exception-throwing validation
> methods, e.g.
>
> ValidateNonNull (name, "name");
> ValidateRange (siteId > 0, "siteId");
>
> etc
>
> --
> Jon Skeet - <s***@pobox.comhttp://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet
> If replying to the group, please do not mail me too

SpecSharp looks very interesting; my problem is that I'm dealing with
lots of VB.NET code, so I'm not sure how useful it will be. I'll keep
looking at it.

The extra items on the stack trace don't really bug me. I think that
the added value of clearer code offsets that tremendously (but that's
just my opinion, and is subject, per usual, to the usual rules
governing opinions).

The "framework" I'm developing does, in fact throw standard exceptions
if the tests don't pass. For example, the standard validator class for
a connection looks like this:

Option Explicit On
Option Strict On

Imports System.Data

Namespace Validation

    Public Class ConnectionValidator
      Inherits ParameterValidatorBase

      Friend Sub New(ByVal connection As IDbConnection, ByVal name As
String)
        MyBase.New(connection, name)
      End Sub

      Public Sub IsNotNull()
        If Connection Is Nothing Then
          Throw New ArgumentNullException(Name)
        End If
      End Sub

      Public Sub IsOpen()
         If Not Connection Is Nothing Then
            If (Connection.State And ConnectionState.Open) = 0 Then
               Throw New _
                 ArgumentException("Operation requires an open
connection.", _
                   Name)
            End If
         End If
      End Sub

      Public Sub IsOpenAndIsNotNull()
         IsNotNull()
         IsOpen()
      End Sub

      Private ReadOnly Property Connection() As IDbConnection
         Get
            Return DirectCast(InnerValue, IDbConnection)
         End Get
      End Property

   End Class

End Namespace

A helper class simplifies invocation as follows:

Option Explicit On
Option Strict On

Imports System.Data.SqlClient

Namespace Validation

    Public Class Validate

      Public Shared Function That(ByVal value As IDbConnection, _
        ByVal parameterName As String) As ConnectionValidator

        Return New ConnectionValidator(value, parameterName)

      End Function

    End Class

End Namespace

(I've snipped many of the other methods to simplify the sample.) While
this class isn't very extensible in it's current incarnation, it's
just a proof of concept to verify that it works. (I'm going for The
Simplest Thing That Works at this point, just to verify that the
interface I want works.)

The end result is code that looks like this:

Validate.That(connection, "connection").IsValid()

which tests that the connection is not null and is open. If either
rule fails, an appropriate exception is thrown, with minimal overhead
on the stack trace.

I've got different validator classes for each of the known core data
types (boolean, byte, char, date, decimal, double, integer, long,
etc.) and the database classes at this point. I also included one for
Enum and Object. These are the classes I use most commonly. The only
problem I'm running into is how the heck to make it extensible so that
validators can be created in other libraries and invoked via the
Validate class, making it extensible at run-time. I know that the
current implementation won't work that way. I'm thinking that I'll
have to make it open-source, which would allow developers to extend it
at their leisure, but that still doesn't present the best solution.

It's been a long day, and I'm just having a tough time wrapping my
brain around the extensibility aspects of it. :-)

Overall, however, I'm *really* liking the effects its having on the
refactored test code. Once the validators are debugged, they work
*exceptionally* well (pardon the pun).

3 May 2007 11:49 PM

Tom Leylan

Hi Mike: I like the basic idea. If I'm reading it right I have a couple of
ideas.

First, note that when you pass a connection you don't send a property. It
can be considered implied (or simply not needed) but it results in the
syntax being different. If there is an "open" property the syntax could be
changed to Validate.That( connection, "Open").IsTrue() to maintain an
"object, property" style. Without it do you introduce more situations where
the developer needs to stop and think "what is the test, what are the
parameters?"

Alternatively the siteId example could be: Validate.That(
siteId ).SiteIdIsValid()? This trades off having to send a second parameter
for more methods. Frankly I'd rather have more methods than have to send
hard to remember (and strings at that) property names. Your
IsOpenAndIsNotNull example demonstrates that a test doesn't need to be
applied to a particular property. I also would (probably) (as a developer)
not want to have to remember all the validation tests that apply to the
"siteId". I'd like to know if it was correct or not but perhaps not want to
have to remember to issue the IsNotNull() test in advance of some other
tests.

The buildingIdToIgnore example is suspect as it places "business logic" into
the object user's hands. Should people testing whether the BuildingId is
valid be required to know that it should be greater than -2? If
IsGreaterThan is a method that the buildingIdToIgnore class should provide
why wouldn't it just report back "it isn't valid"? So it reports "yes" it
is greater than 2 but the rule is it has to be less than 5 as well.

Finally... you may not want to pass a particular object (and a property) in
every case unless the only use is to check the current state of an active
object. It seems to me that testing values "prior to assignment" would be
helpful as well. So the UI developer could check if -5 is a valid
buildingIdToIgnore before actually assigning it. The rules apply to an
instance of an object but also to anybody who might like to know if a
particular value is "valid".

Does this help at all?

Tom

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote...

> Essentially, it's based on NUnit asserts to validate parameters
> consistently and clearly. So, the code above is changed to this:
>
> Validate.That(siteId, "siteId").IsNotNegative()
> Validate.That(name, "name").IsNotNullOrEmpty()
> Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
> Validate.That(connection).IsOpenAndIsNotNull()

4 May 2007 1:05 PM

KKS

In my opinion you have to think of performance as well. Calling a lot of
methods will kill the performance of your original method. So even if these
methods are simple and might be inlined you are not guaranteed that they all
will be. Suggestions along the lines as using a database or xml will be even
worse performance wise. My suggestion to you is to use what you had
originally. I don't think it looks ugly at all.

regards
Kjetil Kristoffer Solberg

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
> I've been maintaining some source code, and I've noticed some radical
> inconsistency in the code, particularly in the way that parameters are
> validated. I've noticed that this seems to have been pretty much the
> case throughout my career.
>
> For instance, consider the following block:
>
> If siteId <= 0 Then
>    Throw New ArgumentOutOfRangeException("siteId")
> End If
> If name Is Nothing Then
>    Throw New ArgumentNullException("name")
> End If
> If name = String.Empty Then
>    Throw New ArgumentException("Empty strings are not permitted.",
> "name")
> End If
> If buildingIdToIgnore < -1 Then
>    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> End If
>
> Sometimes, this code is collapsed as follows:
>
>      If siteId <= 0 Then Throw New
> ArgumentOutOfRangeException("siteId")
>      If name Is Nothing Then Throw New ArgumentNullException("name")
>      If name = String.Empty Then Throw New ArgumentException("Empty
> strings are not permitted.", "name")
>      If buildingIdToIgnore < -1 Then Throw New
> ArgumentOutOfRangeException("buildingIdToIgnore")
>
> Sometimes, it's omitted altogether. Somtimes, not all of the
> parameters to the procedure are validated. And in many cases, the
> messages aren't consistent from one parameter validation to the next.
> (Resource files would go a long way towards solving that, but that's a
> whole different thread.)
>
> What I've been pondering for some time is a way to make it easier to
> validate method parameters. My *theory* is that it's a lot of gangly
> code to write If...Then...Throw...End If, which leads to short-cuts
> and, eventually, outright omissions. So, a way to abbreviate that and
> make the code clearer might help to encourage consistent use of
> parameter validation, which should (in theory) improve code quality
> throughout the system.
>
> So I have this model I'm working on, and before I go completely off
> the deep end with it, I thought I'd run it by you to see what you
> thought of it.
>
> Essentially, it's based on NUnit asserts to validate parameters
> consistently and clearly. So, the code above is changed to this:
>
> Validate.That(siteId, "siteId").IsNotNegative()
> Validate.That(name, "name").IsNotNullOrEmpty()
> Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
> Validate.That(connection).IsOpenAndIsNotNull()
>
> My tests for the model are going very well. I think that once I get it
> going, it could provide a very cool, extensible validation framework
> for parameters. The "framework" is in a separate class library, and
> the messages are in a string resource file, providing consistency.
>
> The big caveat, of course, is that there's no guarantee that it solves
> the *enforcement* problem. There's also the problem that FxCop thinks
> I'm not validating parameters when I am.
>
> So the question I have is, is this even worth doing? When an existing
> code base is refactored to use it, it looks much cleaner, and appears
> to be easier to read and maintain. But the question remains: is this a
> real problem, or am I addressing an issue that doesn't really exist?
>
> Your input is greatly appreciated.
>
> Thanks!
>

4 May 2007 2:04 PM

Jon Skeet [C# MVP]

<"KKS" <kks at synergi dot com>> wrote:
> In my opinion you have to think of performance as well. Calling a lot of
> methods will kill the performance of your original method. So even if these
> methods are simple and might be inlined you are not guaranteed that they all
> will be. Suggestions along the lines as using a database or xml will be even
> worse performance wise. My suggestion to you is to use what you had
> originally. I don't think it looks ugly at all.

I disagree completely:

1) The most elegant and simple way should be used wherever performance
hasn't proved itself to be an issue, IMO. Calling a few methods is
unlikely to "kill" the performance of the original method, and if at
any point the method does any IO (file, socket etc) that's almost
certainly going to dwarf the validation costs.

2) Validation code should be robust but without getting in the way of
making it clear to the reader of the code what the "meat" of the method
is doing - what the real purpose is. Spending 12 lines doing validation
where 3 concise, readable lines will do is not a good idea.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

4 May 2007 8:07 PM

Mike Hofer

Show quote

On May 4, 9:05 am, "KKS" <kks at synergi dot com> wrote:
> In my opinion you have to think of performance as well. Calling a lot of
> methods will kill the performance of your original method. So even if these
> methods are simple and might be inlined you are not guaranteed that they all
> will be. Suggestions along the lines as using a database or xml will be even
> worse performance wise. My suggestion to you is to use what you had
> originally. I don't think it looks ugly at all.
>
> regards
> Kjetil Kristoffer Solberg
>
> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>
> news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
>
>
>
> > I've been maintaining some source code, and I've noticed some radical
> > inconsistency in the code, particularly in the way that parameters are
> > validated. I've noticed that this seems to have been pretty much the
> > case throughout my career.
>
> > For instance, consider the following block:
>
> > If siteId <= 0 Then
> >    Throw New ArgumentOutOfRangeException("siteId")
> > End If
> > If name Is Nothing Then
> >    Throw New ArgumentNullException("name")
> > End If
> > If name = String.Empty Then
> >    Throw New ArgumentException("Empty strings are not permitted.",
> > "name")
> > End If
> > If buildingIdToIgnore < -1 Then
> >    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> > End If
>
> > Sometimes, this code is collapsed as follows:
>
> >      If siteId <= 0 Then Throw New
> > ArgumentOutOfRangeException("siteId")
> >      If name Is Nothing Then Throw New ArgumentNullException("name")
> >      If name = String.Empty Then Throw New ArgumentException("Empty
> > strings are not permitted.", "name")
> >      If buildingIdToIgnore < -1 Then Throw New
> > ArgumentOutOfRangeException("buildingIdToIgnore")
>
> > Sometimes, it's omitted altogether. Somtimes, not all of the
> > parameters to the procedure are validated. And in many cases, the
> > messages aren't consistent from one parameter validation to the next.
> > (Resource files would go a long way towards solving that, but that's a
> > whole different thread.)
>
> > What I've been pondering for some time is a way to make it easier to
> > validate method parameters. My *theory* is that it's a lot of gangly
> > code to write If...Then...Throw...End If, which leads to short-cuts
> > and, eventually, outright omissions. So, a way to abbreviate that and
> > make the code clearer might help to encourage consistent use of
> > parameter validation, which should (in theory) improve code quality
> > throughout the system.
>
> > So I have this model I'm working on, and before I go completely off
> > the deep end with it, I thought I'd run it by you to see what you
> > thought of it.
>
> > Essentially, it's based on NUnit asserts to validate parameters
> > consistently and clearly. So, the code above is changed to this:
>
> > Validate.That(siteId, "siteId").IsNotNegative()
> > Validate.That(name, "name").IsNotNullOrEmpty()
> > Validate.That(buildingIdToIgnore,
> > "buildingIdToIgnore").IsGreaterThan(-2)
> > Validate.That(connection).IsOpenAndIsNotNull()
>
> > My tests for the model are going very well. I think that once I get it
> > going, it could provide a very cool, extensible validation framework
> > for parameters. The "framework" is in a separate class library, and
> > the messages are in a string resource file, providing consistency.
>
> > The big caveat, of course, is that there's no guarantee that it solves
> > the *enforcement* problem. There's also the problem that FxCop thinks
> > I'm not validating parameters when I am.
>
> > So the question I have is, is this even worth doing? When an existing
> > code base is refactored to use it, it looks much cleaner, and appears
> > to be easier to read and maintain. But the question remains: is this a
> > real problem, or am I addressing an issue that doesn't really exist?
>
> > Your input is greatly appreciated.
>
> > Thanks!- Hide quoted text -
>
> - Show quoted text -

Performance is a concern; it's one of the reasons I ruled out
reflection as a way to do it, and also pitched XML templates (parsing
the DOM tends to be slow). I'm also not doing any file IO or database
access.

The method implementations are as small, compact, and simple as can
be. They perform a concrete test and, if the test fails, throw an
exception right away.

The first implementation actually used lots of individual objects for
each test. Each test was represented by a test class that implemented
an interface, and the test was passed to a method that implemented the
interface's sole method.

However, that resulted in a lot of objects being created. I didn't
want to negatively impact performance that way, so I simply settled
for a sealed class that has static methods that return the validator
classes. (It's essentially a factory; the That method is overloaded to
provide strongly typed versions that prevent boxing and unboxing of
value types).

The validator classes are designed around the base data types and the
interfaces I find I commonly use. (Wherever possible, I use interfaces
instead of concrete classes.) The methods on the validator classes are
very small, and simply perform the test, and throw an exception if it
fails, using standard error messages that are stored in a resource
file, providing a high degree of consistency.

If I need to perform multiple tests on a single parameter in VB, I can
now do this:

With Validate.That(myParameter, "myParameter")
.IsNotNull()
.IsInRange(minValue, maxValue)
End With

So yes, performance is a consideration. I haven't forgotten that. And
if I can find any other ways to improve it, I certainly will. I'm open
to suggestions, so fire away!

4 May 2007 9:03 PM

Jon Skeet [C# MVP]

Mike Hofer <kchighl***@gmail.com> wrote:

<snip>

> Performance is a concern; it's one of the reasons I ruled out
> reflection as a way to do it, and also pitched XML templates (parsing
> the DOM tends to be slow). I'm also not doing any file IO or database
> access.

Reflection I can see being a potentially significant performance hit -
but I can't see that a couple of extra method calls which don't need to
do a lot in the success case will cause much performance loss.

You could always profile it, but unless you're really doing *very*
little "real" work in the methods, *and* calling them very often
(really, really often) I doubt you'd see anything.

You could always make the methods compile-time conditional (using
ConditionalAttribute), so that it's easy to repeatedly do performance
tests with and without any validation. That should make it easy to
determine whether or not the impact is significant.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

4 May 2007 10:34 PM

Mike Hofer

On May 4, 5:03 pm, Jon Skeet [C# MVP] <s***@pobox.com> wrote:
Show quote

> Mike Hofer <kchighl***@gmail.com> wrote:
>
> <snip>
>
> > Performance is a concern; it's one of the reasons I ruled out
> > reflection as a way to do it, and also pitched XML templates (parsing
> > the DOM tends to be slow). I'm also not doing any file IO or database
> > access.
>
> Reflection I can see being a potentially significant performance hit -
> but I can't see that a couple of extra method calls which don't need to
> do a lot in the success case will cause much performance loss.
>
> You could always profile it, but unless you're really doing *very*
> little "real" work in the methods, *and* calling them very often
> (really, really often) I doubt you'd see anything.
>
> You could always make the methods compile-time conditional (using
> ConditionalAttribute), so that it's easy to repeatedly do performance
> tests with and without any validation. That should make it easy to
> determine whether or not the impact is significant.
>
> --
> Jon Skeet - <s***@pobox.comhttp://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet
> If replying to the group, please do not mail me too

Thanks for the advice, Jon. I haven't noticed any significant
performance hit so far. Like I said, the functions are lean and mean,
and don't do anything except test and throw on failure. Unless I'm
doing a lot of recursion (not the case yet!), I don't see a problem.

I will run it through a profiler once I get it all set just so I can
see the difference. I'm fairly certain that a lot of this stuff can be
inlined, so it shouldn't be all that costly.

4 May 2007 1:54 PM

Nicole Calinoiu

When validating method arguments, it's important that any ArgumentException
(and subclass) instances appear to have been thrown directly from the target
method. If you defer validation to a helper method, the call stack will
make it look like the ArgumentException was thrown by the helper method in
response to a call from the original target method, and the calling
developer will assume the problem is your fault rather than his.

Obviously, it can be quite helpful to defer complex validation to helper
methods, but any resulting ArgumentException should still be thrown from the
target method. e.g.:

If Not Validate.That(connection).IsOpenAndIsNotNull() Then Throw New
ArgumentException...

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
> I've been maintaining some source code, and I've noticed some radical
> inconsistency in the code, particularly in the way that parameters are
> validated. I've noticed that this seems to have been pretty much the
> case throughout my career.
>
> For instance, consider the following block:
>
> If siteId <= 0 Then
>    Throw New ArgumentOutOfRangeException("siteId")
> End If
> If name Is Nothing Then
>    Throw New ArgumentNullException("name")
> End If
> If name = String.Empty Then
>    Throw New ArgumentException("Empty strings are not permitted.",
> "name")
> End If
> If buildingIdToIgnore < -1 Then
>    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> End If
>
> Sometimes, this code is collapsed as follows:
>
>      If siteId <= 0 Then Throw New
> ArgumentOutOfRangeException("siteId")
>      If name Is Nothing Then Throw New ArgumentNullException("name")
>      If name = String.Empty Then Throw New ArgumentException("Empty
> strings are not permitted.", "name")
>      If buildingIdToIgnore < -1 Then Throw New
> ArgumentOutOfRangeException("buildingIdToIgnore")
>
> Sometimes, it's omitted altogether. Somtimes, not all of the
> parameters to the procedure are validated. And in many cases, the
> messages aren't consistent from one parameter validation to the next.
> (Resource files would go a long way towards solving that, but that's a
> whole different thread.)
>
> What I've been pondering for some time is a way to make it easier to
> validate method parameters. My *theory* is that it's a lot of gangly
> code to write If...Then...Throw...End If, which leads to short-cuts
> and, eventually, outright omissions. So, a way to abbreviate that and
> make the code clearer might help to encourage consistent use of
> parameter validation, which should (in theory) improve code quality
> throughout the system.
>
> So I have this model I'm working on, and before I go completely off
> the deep end with it, I thought I'd run it by you to see what you
> thought of it.
>
> Essentially, it's based on NUnit asserts to validate parameters
> consistently and clearly. So, the code above is changed to this:
>
> Validate.That(siteId, "siteId").IsNotNegative()
> Validate.That(name, "name").IsNotNullOrEmpty()
> Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
> Validate.That(connection).IsOpenAndIsNotNull()
>
> My tests for the model are going very well. I think that once I get it
> going, it could provide a very cool, extensible validation framework
> for parameters. The "framework" is in a separate class library, and
> the messages are in a string resource file, providing consistency.
>
> The big caveat, of course, is that there's no guarantee that it solves
> the *enforcement* problem. There's also the problem that FxCop thinks
> I'm not validating parameters when I am.
>
> So the question I have is, is this even worth doing? When an existing
> code base is refactored to use it, it looks much cleaner, and appears
> to be easier to read and maintain. But the question remains: is this a
> real problem, or am I addressing an issue that doesn't really exist?
>
> Your input is greatly appreciated.
>
> Thanks!
>

4 May 2007 2:14 PM

Jon Skeet [C# MVP]

<"Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT com>> wrote:
> When validating method arguments, it's important that any ArgumentException
> (and subclass) instances appear to have been thrown directly from the target
> method.

I'm not sure that's particularly important. I'd say it's a "nice to
have" but I wouldn't make the validation code harder to ensure it. The
framework certainly doesn't. For instance:

using System;
using System.Collections.Generic;

class Test
{
    static void Main()
    {
        try
        {
            List<string> x = new List<string>();
            x[5] = null;
        }
        catch (Exception e)
        {
            Console.WriteLine (e);
        }
    }
}

displays (on my box):

System.ArgumentOutOfRangeException: Index was out of range. Must be
non-negative
and less than the size of the collection.
Parameter name: index
   at System.ThrowHelper.ThrowArgumentOutOfRangeException
(ExceptionArgument argument, ExceptionResource resource)
   at System.ThrowHelper.ThrowArgumentOutOfRangeException()
   at System.Collections.Generic.List`1.set_Item(Int32 index, T value)
   at Test.Main()

There are two more levels than one might expect. I don't think that's
particularly confusing, so long as the message is very clear.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

5 May 2007 11:29 AM

Nicole Calinoiu

"Jon Skeet [C# MVP]" <sk***@pobox.com> wrote in message
news:MPG.20a55004e6b0d77e5b@msnews.microsoft.com...
> I'm not sure that's particularly important. I'd say it's a "nice to
> have" but I wouldn't make the validation code harder to ensure it.

How important it might be really depends on the intended audience for your
API. If you're developing librairies for internal use within an
organization, and you have the opportunity to communicate directly with the
API users, then it may not be very important at all. However, if you're
producing an API for truly public use and can expect to have very
inexperienced developers as clients, then it may have a much greater impact.

> The framework certainly doesn't.

Actually, in most places in the framework, the developers have gone to the
trouble of throwing directly from the method whose arguments are being
validated (see, for example, ArrayList.Item). The List<T>.Item
counter-example should probably be considered a bug. In fact, it's even
worse than you might expect since it's not actually deferring validation to
the helper method, but only the exception creation
and throwing ("If (index >= Me._size) Then
ThrowHelper.ThrowArgumentOutOfRangeException"). I suspect that the
developers might have taken the "consider using exception builder methods"
design guideline (see
http://msdn2.microsoft.com/en-us/library/ms229030.aspx) a little too far in
this particular case. At any rate, whatever the reason, the fact that
someone working on the BCL made what almost certainly amounts to a mistake
shouldn't be justification for the rest of us to get lazy. ;)

> There are two more levels than one might expect. I don't think that's
> particularly confusing, so long as the message is very clear.

If you were the only client of my APIs, I wouldn't worry about where the
exceptions were thrown either. :)

5 May 2007 11:54 AM

Nicole Calinoiu

"Jon Skeet [C# MVP]" <sk***@pobox.com> wrote in message
news:MPG.20a55004e6b0d77e5b@msnews.microsoft.com...
> I'm not sure that's particularly important. I'd say it's a "nice to
> have" but I wouldn't make the validation code harder to ensure it.

How important it might be really depends on the intended audience for your
API. If you're developing librairies for internal use within an
organization, and you have the opportunity to communicate directly with the
API users, then it may not be very important at all. However, if you're
producing an API for truly public use and can expect to have very
inexperienced developers as clients, then it may have a much greater impact.

> The framework certainly doesn't.

Actually, in most places in the framework, the developers have gone to the
trouble of throwing directly from the method whose arguments are being
validated (see, for example, ArrayList.Item). The List<T>.Item
counter-example should probably be considered a bug. In fact, it's even
worse than you might expect since it's not actually deferring validation to
the helper method, but only the exception creation
and throwing ("If (index >= Me._size) Then
ThrowHelper.ThrowArgumentOutOfRangeException"). I suspect that the
developers might have taken the "consider using exception builder methods"
design guideline (see
http://msdn2.microsoft.com/en-us/library/ms229030.aspx) a little too far in
this particular case. At any rate, whatever the reason, the fact that
someone working on the BCL made what almost certainly amounts to a mistake
shouldn't be justification for the rest of us to get lazy. ;)

> There are two more levels than one might expect. I don't think that's
> particularly confusing, so long as the message is very clear.

If you were the only client of my APIs, I wouldn't worry about where the
exceptions were thrown either. :)

4 May 2007 7:31 PM

Mike Hofer

On May 4, 9:54 am, "Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT
com> wrote:
Show quote

> When validating method arguments, it's important that any ArgumentException
> (and subclass) instances appear to have been thrown directly from the target
> method. If you defer validation to a helper method, the call stack will
> make it look like the ArgumentException was thrown by the helper method in
> response to a call from the original target method, and the calling
> developer will assume the problem is your fault rather than his.
>
> Obviously, it can be quite helpful to defer complex validation to helper
> methods, but any resulting ArgumentException should still be thrown from the
> target method. e.g.:
>
> If Not Validate.That(connection).IsOpenAndIsNotNull() Then Throw New
> ArgumentException...
>
> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>
> news:1178207546.603734.255160@l77g2000hsb.googlegroups.com...
>
>
>
> > I've been maintaining some source code, and I've noticed some radical
> > inconsistency in the code, particularly in the way that parameters are
> > validated. I've noticed that this seems to have been pretty much the
> > case throughout my career.
>
> > For instance, consider the following block:
>
> > If siteId <= 0 Then
> >    Throw New ArgumentOutOfRangeException("siteId")
> > End If
> > If name Is Nothing Then
> >    Throw New ArgumentNullException("name")
> > End If
> > If name = String.Empty Then
> >    Throw New ArgumentException("Empty strings are not permitted.",
> > "name")
> > End If
> > If buildingIdToIgnore < -1 Then
> >    Throw New ArgumentOutOfRangeException("buildingIdToIgnore")
> > End If
>
> > Sometimes, this code is collapsed as follows:
>
> >      If siteId <= 0 Then Throw New
> > ArgumentOutOfRangeException("siteId")
> >      If name Is Nothing Then Throw New ArgumentNullException("name")
> >      If name = String.Empty Then Throw New ArgumentException("Empty
> > strings are not permitted.", "name")
> >      If buildingIdToIgnore < -1 Then Throw New
> > ArgumentOutOfRangeException("buildingIdToIgnore")
>
> > Sometimes, it's omitted altogether. Somtimes, not all of the
> > parameters to the procedure are validated. And in many cases, the
> > messages aren't consistent from one parameter validation to the next.
> > (Resource files would go a long way towards solving that, but that's a
> > whole different thread.)
>
> > What I've been pondering for some time is a way to make it easier to
> > validate method parameters. My *theory* is that it's a lot of gangly
> > code to write If...Then...Throw...End If, which leads to short-cuts
> > and, eventually, outright omissions. So, a way to abbreviate that and
> > make the code clearer might help to encourage consistent use of
> > parameter validation, which should (in theory) improve code quality
> > throughout the system.
>
> > So I have this model I'm working on, and before I go completely off
> > the deep end with it, I thought I'd run it by you to see what you
> > thought of it.
>
> > Essentially, it's based on NUnit asserts to validate parameters
> > consistently and clearly. So, the code above is changed to this:
>
> > Validate.That(siteId, "siteId").IsNotNegative()
> > Validate.That(name, "name").IsNotNullOrEmpty()
> > Validate.That(buildingIdToIgnore,
> > "buildingIdToIgnore").IsGreaterThan(-2)
> > Validate.That(connection).IsOpenAndIsNotNull()
>
> > My tests for the model are going very well. I think that once I get it
> > going, it could provide a very cool, extensible validation framework
> > for parameters. The "framework" is in a separate class library, and
> > the messages are in a string resource file, providing consistency.
>
> > The big caveat, of course, is that there's no guarantee that it solves
> > the *enforcement* problem. There's also the problem that FxCop thinks
> > I'm not validating parameters when I am.
>
> > So the question I have is, is this even worth doing? When an existing
> > code base is refactored to use it, it looks much cleaner, and appears
> > to be easier to read and maintain. But the question remains: is this a
> > real problem, or am I addressing an issue that doesn't really exist?
>
> > Your input is greatly appreciated.
>
> > Thanks!- Hide quoted text -
>
> - Show quoted text -

I'm not sure I agree wholeheartedly with this one, though I understand
the concern. The question is one of risk mitigation, and whether or
not the trade-off in code clarity is worth the additional entries in
the stack trace.

I tend to agree with Jon. Other classes in the framework add
additional entries to the stack trace. However, the fact that the
validation methods in this case only add two entries, and both of them
indicate that they are Validation failures, makes it clear that we're
dealing with a parameter validation. The classes and methods are
*very* clearly named. I *think* (dangerous word, I know) that any
competent programmer would be able to discern that the problem in
question was that the validation test had failed.

Further, one of the points of the framework is to refactor complex
validation code so that it's *easier* to do, encouraging its
implementation. If I do it the way you've shown, it doesn't appear
that it would be easier to do. I might even argue that it would be
harder, and the additional overhead in code would buy me nothing. In
that event, I might as well just step away from the keyboard, and drop
the project.

My goals here are to provide a streamlined means of validating
parameters that results in a consistent set of error messages, clear
code, with a minimal impact on the stack trace. I don't think that any
framework is going to be able to provide that without impacting the
stack trace, without resulting in lots of code at the point of call,
which defeats its purpose. I could be wrong, and am certainly willing
to entertain that notion.

I am definitely interested in a differing viewpoint. It may prove to
be true that the system would benefit from *both* ways of doing
things. So by all means, come back with a response, and help me to
build something great.

Thanks!

5 May 2007 12:10 PM

Nicole Calinoiu

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178307104.284053.190140@h2g2000hsg.googlegroups.com...
> I'm not sure I agree wholeheartedly with this one, though I understand
> the concern. The question is one of risk mitigation, and whether or
> not the trade-off in code clarity is worth the additional entries in
> the stack trace.

That's certainly true, and only you can decide which way the trade-off tips
for your particular API. However, the larger and more diverse the intended
developer audience for your API, the more emphasis you should be placing on
the interests of the API users versus those of the API developers.

> I tend to agree with Jon. Other classes in the framework add
> additional entries to the stack trace.

See my response to Jon. The cases where the framework does this are
probably not desirable.

> However, the fact that the
> validation methods in this case only add two entries, and both of them
> indicate that they are Validation failures, makes it clear that we're
> dealing with a parameter validation. The classes and methods are
> *very* clearly named. I *think* (dangerous word, I know) that any
> competent programmer would be able to discern that the problem in
> question was that the validation test had failed.

Are you sure that all the developers in your intended API audience are all
that competent, experienced, etc.? If so, then you have no worries.

> Further, one of the points of the framework is to refactor complex
> validation code so that it's *easier* to do, encouraging its
> implementation.

Where do you get that idea? If encouraging argument validation was a
serious goal for the folks on the CLR or language teams at Microsoft,
declarative validation would have made its way out of the Spec# research pit
quite some time ago, and they would never have permitted the introduction of
automatic properties without support for declarative validation. Heck, they
could have done as little as salvaging the XC# post-compiler
(http://www.resolvecorp.com), which seems to have suffered a largely ignored
demise sometime before Visual Studio 2005 was released.

Personally, I would argue that there is next to no framework support for
doing the right thing with respect to argument validation, but then I might
just be a bit bitter... ;)

> If I do it the way you've shown, it doesn't appear
> that it would be easier to do. I might even argue that it would be
> harder, and the additional overhead in code would buy me nothing.

Again, if it actually buys you nothing because your intended audience are
sufficiently sophisticated, then it's probably not worthwhile. However,
it's not actually all that much more work to put the throws in the target
methods. Granted, it's irritating (there is a reason I'm bitter <g>), but
it's not exactly expensive.

> My goals here are to provide a streamlined means of validating
> parameters that results in a consistent set of error messages, clear
> code, with a minimal impact on the stack trace. I don't think that any
> framework is going to be able to provide that without impacting the
> stack trace, without resulting in lots of code at the point of call,
> which defeats its purpose. I could be wrong, and am certainly willing
> to entertain that notion.

There's a way to avoid the impact on the stack trace, but it's terribly
inelegant and would require adding more code to the target method. However,
just for the sake of completeness, here it is...

Since the call stack for an exception is generated when it is thrown, your
target method would wrap its validation method calls in a try catch that
explicitly rethrows any caught argument exceptions. e.g.:

Try
    Validate.That(siteId, "siteId").IsNotNegative()
    Validate.That(name, "name").IsNotNullOrEmpty()
    Validate.That(buildingIdToIgnore,
"buildingIdToIgnore").IsGreaterThan(-2)
    Validate.That(connection).IsOpenAndIsNotNull()
Catch ex As ArgumentException
    Throw ex
End Try

Personally, I think it's far less palatable than the alternatives, but
ymmv...

5 May 2007 2:43 PM

Jon Skeet [C# MVP]

<"Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT com>> wrote:

<snip>

> Personally, I would argue that there is next to no framework support for
> doing the right thing with respect to argument validation, but then I might
> just be a bit bitter... ;)

I agree wholeheartedly. Apart from the sad lack of Spec# progression,
it's a shame there's no way of applying an attribute to a method to say
"any exceptions thrown by this method should appear as if they're
thrown by the caller" which would make things a lot simpler.

I think it's just a balance thing - I think it's likely that if we ask
developers to do validation "the long way", they may well not bother,
and that's worse than exceptions having a couple of levels of stack too
many.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

7 May 2007 11:12 AM

Mike Hofer

On May 5, 8:10 am, "Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT
com> wrote:

> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>
> news:1178307104.284053.190140@h2g2000hsg.googlegroups.com...
>
> > I'm not sure I agree wholeheartedly with this one, though I understand
> > the concern. The question is one of risk mitigation, and whether or
> > not the trade-off in code clarity is worth the additional entries in
> > the stack trace.
>
> That's certainly true, and only you can decide which way the trade-off tips
> for your particular API. However, the larger and more diverse the intended
> developer audience for your API, the more emphasis you should be placing on
> the interests of the API users versus those of the API developers.

That's a very valid point, and one well worth consideration. Even if
the code is well documented, and the end-user documentation is clear,
a user who is simply testing code may be in for a surprise when an
exception is thrown by the library and they've never worked with it
before. It's something I hadn't thought of before.

It's a shame there's no way to build or modify the stack trace to
remove the entries so that they accurate reflect the offending
routine.

Show quote

>
> > I tend to agree with Jon. Other classes in the framework add
> > additional entries to the stack trace.
>
> See my response to Jon. The cases where the framework does this are
> probably not desirable.
>
> > However, the fact that the
> > validation methods in this case only add two entries, and both of them
> > indicate that they are Validation failures, makes it clear that we're
> > dealing with a parameter validation. The classes and methods are
> > *very* clearly named. I *think* (dangerous word, I know) that any
> > competent programmer would be able to discern that the problem in
> > question was that the validation test had failed.
>
> Are you sure that all the developers in your intended API audience are all
> that competent, experienced, etc.? If so, then you have no worries.

I am expecting a *certain* degree of competence. I would expect that
anyone using this library is looking for a way to improve code
quality, much like a user who seeks out NUnit to improve code quality
through the use of TDD. However, as you've said, it's certainly going
to be interesting to see what happens when users look at the thing and
they've never seen it before, and the stack trace contains additional
entries.

Show quote

>
> > Further, one of the points of the framework is to refactor complex
> > validation code so that it's *easier* to do, encouraging its
> > implementation.
>
> Where do you get that idea? If encouraging argument validation was a
> serious goal for the folks on the CLR or language teams at Microsoft,
> declarative validation would have made its way out of the Spec# research pit
> quite some time ago, and they would never have permitted the introduction of
> automatic properties without support for declarative validation. Heck, they
> could have done as little as salvaging the XC# post-compiler
> (http://www.resolvecorp.com), which seems to have suffered a largely ignored
> demise sometime before Visual Studio 2005 was released.
>
> Personally, I would argue that there is next to no framework support for
> doing the right thing with respect to argument validation, but then I might
> just be a bit bitter... ;)

Whoops! That was my bad! By "framework" I was referring to the
framework that I am developing. I usually refer to the .NET Framework
with "Framework" (capital F). (I'm a spelling nazi, but always forget
that this is the Internet, and it doesn't universally apply, and
others may not pick up on that.)

What I should have said was that the purpose of the framework that I
am developing is to refactor complex code. The .NET Framework itself
almost certainly *never* does this, as you well know, but gives you
the raw building blocks, and then you have to refactor the hell out of
it.

My apologies for not being clear on that.

Show quote

>
> > If I do it the way you've shown, it doesn't appear
> > that it would be easier to do. I might even argue that it would be
> > harder, and the additional overhead in code would buy me nothing.
>
> Again, if it actually buys you nothing because your intended audience are
> sufficiently sophisticated, then it's probably not worthwhile. However,
> it's not actually all that much more work to put the throws in the target
> methods. Granted, it's irritating (there is a reason I'm bitter <g>), but
> it's not exactly expensive.
>
> > My goals here are to provide a streamlined means of validating
> > parameters that results in a consistent set of error messages, clear
> > code, with a minimal impact on the stack trace. I don't think that any
> > framework is going to be able to provide that without impacting the
> > stack trace, without resulting in lots of code at the point of call,
> > which defeats its purpose. I could be wrong, and am certainly willing
> > to entertain that notion.
>
> There's a way to avoid the impact on the stack trace, but it's terribly
> inelegant and would require adding more code to the target method. However,
> just for the sake of completeness, here it is...
>
> Since the call stack for an exception is generated when it is thrown, your
> target method would wrap its validation method calls in a try catch that
> explicitly rethrows any caught argument exceptions. e.g.:
>
> Try
>     Validate.That(siteId, "siteId").IsNotNegative()
>     Validate.That(name, "name").IsNotNullOrEmpty()
>     Validate.That(buildingIdToIgnore,
> "buildingIdToIgnore").IsGreaterThan(-2)
>     Validate.That(connection).IsOpenAndIsNotNull()
> Catch ex As ArgumentException
>     Throw ex
> End Try
>
> Personally, I think it's far less palatable than the alternatives, but
> ymmv...

Just brainstorming here, but let me run this by you. Since I can't
modify the stack trace itself, and since I don't want additional
information in it, what if I used a custom exception class, such as a
ParameterValidtationException, and that class shadowed the StackTrace
property. I could then rewrite the stack trace so that I could fetch
it, remove the extra entries, and then expose the cleaned list to the
end user? Further, I could then extend that class with specific
exception types, if need be, to mimic the standard argument
exceptions.

It might have several advantages: most notably, it keeps the stack
trace clean, but it also tells you explicitly that the exception was
thrown by the validation framework. With some optimization, the code
to clean the stack trace can be kept clean, small, and fast.

Thoughts?

7 May 2007 12:16 PM

KKS

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178536364.814368.45380@h2g2000hsg.googlegroups.com...
> Just brainstorming here, but let me run this by you. Since I can't
> modify the stack trace itself, and since I don't want additional
> information in it, what if I used a custom exception class, such as a
> ParameterValidtationException, and that class shadowed the StackTrace
> property. I could then rewrite the stack trace so that I could fetch
> it, remove the extra entries, and then expose the cleaned list to the
> end user? Further, I could then extend that class with specific
> exception types, if need be, to mimic the standard argument
> exceptions.
>
> It might have several advantages: most notably, it keeps the stack
> trace clean, but it also tells you explicitly that the exception was
> thrown by the validation framework. With some optimization, the code
> to clean the stack trace can be kept clean, small, and fast.
>
> Thoughts?
>

Is this really that important? It seems to me now that you creating a lot of
work for yourself. Just my opinion.

Regards
Kjetil

7 May 2007 9:34 PM

Mike Hofer

Show quote

On May 7, 8:16 am, "KKS" <kks at synergi dot com> wrote:
> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>
> news:1178536364.814368.45380@h2g2000hsg.googlegroups.com...
>
>
>
> > Just brainstorming here, but let me run this by you. Since I can't
> > modify the stack trace itself, and since I don't want additional
> > information in it, what if I used a custom exception class, such as a
> > ParameterValidtationException, and that class shadowed the StackTrace
> > property. I could then rewrite the stack trace so that I could fetch
> > it, remove the extra entries, and then expose the cleaned list to the
> > end user? Further, I could then extend that class with specific
> > exception types, if need be, to mimic the standard argument
> > exceptions.
>
> > It might have several advantages: most notably, it keeps the stack
> > trace clean, but it also tells you explicitly that the exception was
> > thrown by the validation framework. With some optimization, the code
> > to clean the stack trace can be kept clean, small, and fast.
>
> > Thoughts?
>
> Is this really that important? It seems to me now that you creating a lot of
> work for yourself. Just my opinion.
>
> Regards
> Kjetil- Hide quoted text -
>
> - Show quoted text -

I'm not sure. I just want to make sure that I've covered all my bases.
If enough users think that it would be a big enough concern, then I'd
want to make sure it wasn't going to be a substantial enough deterrant
to prevent its use, and handle that. But I'd only do so if the
implementation didn't destroy the usefulness of the library.

Also, I don't think it would be that much work to do it, especially if
it provided clarity to the end-user. Clarity, in my eyes, is a Very
Good Thing, and it's worth the up-front effort on my part if it
provides a larger return on my investment down the road. Not just for
myself, mind you, but for everyone who might use it.

But again, therein lies the rub: Does it provide clarity and
usefulness? If it does, then I'd do it. If it doesn't, then I wouldn't.

8 May 2007 1:37 PM

Nicole Calinoiu

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178536364.814368.45380@h2g2000hsg.googlegroups.com...
> I am expecting a *certain* degree of competence. I would expect that
> anyone using this library is looking for a way to improve code
> quality, much like a user who seeks out NUnit to improve code quality
> through the use of TDD.

What about their API users, who will be the ultimate consumers of exceptions
generated from your framework? Besides potentially not fitting the expected
profile for users of your library, they'll also be blissfully unaware that
your validation framework is even being used by the API they are invoking.

> Just brainstorming here, but let me run this by you. Since I can't
> modify the stack trace itself, and since I don't want additional
> information in it, what if I used a custom exception class, such as a
> ParameterValidtationException, and that class shadowed the StackTrace
> property. I could then rewrite the stack trace so that I could fetch
> it, remove the extra entries, and then expose the cleaned list to the
> end user? Further, I could then extend that class with specific
> exception types, if need be, to mimic the standard argument
> exceptions.

Unfortunately, swapping out the exception type is going to be a breaking
change for existing code. For example, an existing catch block that handles
System.ArgumentNullException won't catch your ParameterValidationException
or a derived
ParameterNullException. You could work around this by subclassing the
built-in specific exception types and adding your stack trace manipulation
via composition.

> It might have several advantages: most notably, it keeps the stack
> trace clean, but it also tells you explicitly that the exception was
> thrown by the validation framework.

Users of the APIs produced by users of your framework probably won't want to
see any difference between the exceptions it throws and any other argument
exceptions they might need to handle.

> With some optimization, the code
> to clean the stack trace can be kept clean, small, and fast.

And able to handle stack traces generated in languages other than English?

8 May 2007 4:55 PM

Mike Hofer

On May 8, 9:37 am, "Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT
com> wrote:

> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>
> news:1178536364.814368.45380@h2g2000hsg.googlegroups.com...
>
> > I am expecting a *certain* degree of competence. I would expect that
> > anyone using this library is looking for a way to improve code
> > quality, much like a user who seeks out NUnit to improve code quality
> > through the use of TDD.
>
> What about their API users, who will be the ultimate consumers of exceptions
> generated from your framework? Besides potentially not fitting the expected
> profile for users of your library, they'll also be blissfully unaware that
> your validation framework is even being used by the API they are invoking.

Third, fourth, and fifth generation users, then? I hadn't thought of
that. Good catch.

Show quote

> > Just brainstorming here, but let me run this by you. Since I can't
> > modify the stack trace itself, and since I don't want additional
> > information in it, what if I used a custom exception class, such as a
> > ParameterValidtationException, and that class shadowed the StackTrace
> > property. I could then rewrite the stack trace so that I could fetch
> > it, remove the extra entries, and then expose the cleaned list to the
> > end user? Further, I could then extend that class with specific
> > exception types, if need be, to mimic the standard argument
> > exceptions.
>
> Unfortunately, swapping out the exception type is going to be a breaking
> change for existing code. For example, an existing catch block that handles
> System.ArgumentNullException won't catch your ParameterValidationException
> or a derived
> ParameterNullException. You could work around this by subclassing the
> built-in specific exception types and adding your stack trace manipulation
> via composition.

Okay, that works.

> > It might have several advantages: most notably, it keeps the stack
> > trace clean, but it also tells you explicitly that the exception was
> > thrown by the validation framework.
>
> Users of the APIs produced by users of your framework probably won't want to
> see any difference between the exceptions it throws and any other argument
> exceptions they might need to handle.

I'll buy that, with one caveat: In my limited experience, there's
alway the edge case that proves my expectations wrong. I'd at least
want to provide *some* way to let users know that the exception was
thrown from my framework, and not directly from within their code.

> > With some optimization, the code
> > to clean the stack trace can be kept clean, small, and fast.
>
> And able to handle stack traces generated in languages other than English?

Another very good point. I was actually thinking of making this open
souce once I got the base stuff done, and since I know little to
nothing about proper globalization (yeah, it's a gaping hole in my
skillset), I figured that others could help me cover that ground. I
don't pretend for an instant that I know everything, or even the vast
majority of all that there is to know about the proper or best way to
do this. It's why I started this thread in the first place. And your
insights have been invaluable. Thank you very much.

9 May 2007 11:51 AM

Nicole Calinoiu

If you're willing to put a bit of work into this, you might want to consider
going the post-compiler route instead in order to allow declarative
validation. A few years ago, I was thinking of doing some similar stack
trace manipulation when I ran across XC#. Unfortunately, given that you're
targeting VB, you wouldn't be able to use XC# even if it were still
available. However, there does appear to be a more general .NET
post-compiler available at http://www.postsharp.org/. I'll be taking a
closer look at it myself as soon as I get a chance, but it does look like a
reasonable candidate for enabling declarative validation if it lives up to
its promise...

Show quote

"Mike Hofer" <kchighl***@gmail.com> wrote in message
news:1178643318.814918.93500@l77g2000hsb.googlegroups.com...
> On May 8, 9:37 am, "Nicole Calinoiu" <calinoiu REMOVETHIS AT gmail DOT
> com> wrote:
>> "Mike Hofer" <kchighl***@gmail.com> wrote in message
>>
>> news:1178536364.814368.45380@h2g2000hsg.googlegroups.com...
>>
>> > I am expecting a *certain* degree of competence. I would expect that
>> > anyone using this library is looking for a way to improve code
>> > quality, much like a user who seeks out NUnit to improve code quality
>> > through the use of TDD.
>>
>> What about their API users, who will be the ultimate consumers of
>> exceptions
>> generated from your framework? Besides potentially not fitting the
>> expected
>> profile for users of your library, they'll also be blissfully unaware
>> that
>> your validation framework is even being used by the API they are
>> invoking.
>
> Third, fourth, and fifth generation users, then? I hadn't thought of
> that. Good catch.
>
>> > Just brainstorming here, but let me run this by you. Since I can't
>> > modify the stack trace itself, and since I don't want additional
>> > information in it, what if I used a custom exception class, such as a
>> > ParameterValidtationException, and that class shadowed the StackTrace
>> > property. I could then rewrite the stack trace so that I could fetch
>> > it, remove the extra entries, and then expose the cleaned list to the
>> > end user? Further, I could then extend that class with specific
>> > exception types, if need be, to mimic the standard argument
>> > exceptions.
>>
>> Unfortunately, swapping out the exception type is going to be a breaking
>> change for existing code. For example, an existing catch block that
>> handles
>> System.ArgumentNullException won't catch your
>> ParameterValidationException
>> or a derived
>> ParameterNullException. You could work around this by subclassing the
>> built-in specific exception types and adding your stack trace
>> manipulation
>> via composition.
>
> Okay, that works.
>
>> > It might have several advantages: most notably, it keeps the stack
>> > trace clean, but it also tells you explicitly that the exception was
>> > thrown by the validation framework.
>>
>> Users of the APIs produced by users of your framework probably won't want
>> to
>> see any difference between the exceptions it throws and any other
>> argument
>> exceptions they might need to handle.
>
> I'll buy that, with one caveat: In my limited experience, there's
> alway the edge case that proves my expectations wrong. I'd at least
> want to provide *some* way to let users know that the exception was
> thrown from my framework, and not directly from within their code.
>
>> > With some optimization, the code
>> > to clean the stack trace can be kept clean, small, and fast.
>>
>> And able to handle stack traces generated in languages other than
>> English?
>
> Another very good point. I was actually thinking of making this open
> souce once I got the base stuff done, and since I know little to
> nothing about proper globalization (yeah, it's a gaping hole in my
> skillset), I figured that others could help me cover that ground. I
> don't pretend for an instant that I know everything, or even the vast
> majority of all that there is to know about the proper or best way to
> do this. It's why I started this thread in the first place. And your
> insights have been invaluable. Thank you very much.
>