Software Best Practices

Voices on Software Development Best Practices
Welcome to Software Best Practices Sign in | Join | Help
in Search

Fuzziness and Overzealousnes In Parameter Validation

Last post 07-25-2007 8:17 AM by michael. 8 replies.
Page 1 of 1 (9 items)
Sort Posts: Previous Next
  • 07-07-2007 7:57 AM

    Fuzziness and Overzealousnes In Parameter Validation

    Bear with me as I paint a picture here, to provide a little background.

    Background 

    As part of the project I'm working on, I developed a library that cleans up method parameter validation. The idea was simple: by writing less code, I'd be more inclined to do it more consistently.

    We do a lot of work with the database, and we like to prevent bad data from ever getting into the database in the first place. We like to erect multiple walls of protection in order to keep that garbage out of the database. First, there's the UI, where we use validators in the ASP.NET UI. Then, within our application's methods, we validate the parameters as they make their way towards the functions that eventually invoke the stored procedures.

    The problem is, as you can imagine, that makes for a lot of code like this:

       If myUserIdParameter >= 0 Then
          Throw New ArgumentOutOfRangeException("myUserIdParameter must be a positive number.")
       End If 

    In large quantities, code like that gets tedious, and we (by which I mean I) can become disinclined to do it with consistency in the mad rush to get the code out the door. Informal code reviews and defect correction efforts bore this out. So, I went in search of a solution that would make it easier for me to write parameter validation code that would (a) work, (b) enhance the readability of the code, and (c) encourage me to do it consistently because it didn't take a lot of extra code.

    The end result was a code library based on the model used by NUnit (in its earlier days). The resulting validation looks like this:

       Validate.That(myUserIdParameter, "myUserIdParameter").IsPositive()

    The library works very well (though it has some extensibility issues I have yet to work out). My code is very clean, very easy to understand, and I find that I am now far more inclined to validate parameters to methods simply because it's easy to do and takes a far smaller amount of code to do it in.

    The Problems

    I am now encountering two primary issues that are sending up red flags in my head: fuzziness and zeal. In reality, I think they overlap, and the above code snippet is an example.

    It's so easy now to validate parameters, that I am sometimes unclear as to whether or not I am validating a business rule, or throwing an exception because the parameter's value would cause the method to fail. In all cases, a call to Validate.That() should only be done because you expect that the value of the parameter would cause the method to either fail or return invalid results. That's my basic understanding of why you would use parameter validation in the first place. (Someone please correct me if I'm wrong.)

    I use these methods in the following cases:

    • If a parameter is passed that I know would cause a foreign key constraint failure during an insert or update to the database. It helps me to avoid an unnecessary round trip to the database.
    • If a parameter is passed with a primary key that I know is invalid for a SQL Select. This also prevents unnecessary round trips to the database.
    • Invalid SqlConnections (null or not open) and SqlTransactions (null).
    • Negative numbers.
    • Null or empty strings when they aren't allowed.

    Where do you draw the line between things that you should be checking for, and things that you shouldn't? It doesn't always seem clear to me.

    It bears noting that I validate parameters throughout the entire call sequence. So if there are ten calls in the call stack before we get to the Framework itself, I'm validating parameters every step of the way. That's where zeal comes into the picture. I'm worried at this point that I've become obsessive-compulsive about it. On the one hand, I'm pretty confident that I'll know early on if I've goofed up somewhere in my code and passed garbage to a routine that doesn't allow it. On the other, I'm wondering what the performance penalties are going to be like.

    The library I wrote is very small, and very lean. It doesn't create a lot of objects, and it just performs a simple test. If the test passes, it moves merrily along. If the test fails, it throws an appropriately typed exception. The end result is that the code is cleaner at the point of call (and, hopefully, more robust).

    I would have posted this to the Google Newsgroups, but my experience there has been lackluster at best. I'm really hoping to get some serious feedback and guidance on where to draw the line, and on whether or not I need to rethink my approach to the many-layered parameter validation I've got going on. Even if you tell me that the library itself is a phenomenally bad idea, and that I should just suck it up and hand-code the parameter validation like everyone else does.

    Any help you folks can provide will be very much appreciated. 

    Sincerely,

    Mike
     

  • 07-08-2007 4:00 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    Two things should be taken into consideration 

    First: One of the main points in the design is design by responsibilities. In your application, every layer should only check for what relates to its responsibility. For example, if your business rules state that an employee can not access data outside his department then you should check this in the business logic layer only and there is no need to check for this elsewhere. This is important for maintainability and future changes.

    The second point is your error handling technique. Mainly error handling is done by exception handling or method return parameter. Both method can force different strategy. I assume you are using exceptions from what you have said.

    If you are using exception handling, then try to handle exceptions in the top most layer unless necessary. For example in asp.net you can catch the errors in the application on error and log them to the event logs and direct the user to a generic error page. This technique depends on the idea exception propagation to upper layers, and decreases the amount of code written to handle the exceptions. If a specific exception is important to you then you can catch it in the layer that it responsible for this type of exception and handle it.

    Generally speaking, the idea of checking inputs to every method is very much recommended when you are developing libraries that will be used in multiple applications so you should assume nothing about the input, and it is recommended in C++, and mainly check array bounds to avoid buffer overruns. In your case, don't check for inputs in every method unless the problem caused by wrong input is severe (buffer overrun for example). There is no need to check for a foreign key constraint error if you know that an exception will be thrown if the constraint was violated. Also you should calculate how much time would a trip to the database cost you before writing tons of code to avoid it.

    Filed under:
  • 07-09-2007 9:11 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    Maybe if we could review this library we'd have a better idea... ;)

  • 07-09-2007 1:07 PM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    @Michael: 

    Okay, I had to laugh at that.

    Admittedly, it's a work in progress. However, I will gladly post what I have, if there's a decent place to post it. (I've complained about the lack of a unified online Peer Review Forum before. See here.)

    -------------

    In the interests of fostering discussion, here's what I have (edited down). Bear in mind that's VB.NET, and that I haven't gotten around to localizing it into resource files yet. (I figure I'll do that when I get the green light from folks who have determined that it's a worthwhile endeavor.)

    Key principles:

    1.       It had to be SIMPLE.

    2.       It had to be EASY TO USE.

    3.       It had to WORK.

    4.       It had to be FAST.

    5.       It had to be EASY TO UNDERSTAND.

    6.       It had to be EASY TO MAINTAIN.

    The goal of the “library” “framework” or whatever you want to call it is to provide a standard means of validating method parameters that encourages their consistent use throughout the application. The basic premise: parameter validation code tends to be verbose, discouraging me from doing it. By making it more terse, and by taking advantage of IntelliSense, I might be more inclined to do it at the start of every method, or to at least think about it.

    I used NUnit’s Assert.That() statement as a model for the interface, since it was familiar, and the learning curve would be small. Under the hood, however, the mechanics are radically different, since NUnit relies heavily on reflection (not an option for a library where the methods may be called multiple times on every layer of the call stack).

    For a very basic example, we'll look at the ConnectionValidator class. In order to validate a parameter, you invoke the heavily overloaded That method of the Validate class. (For connections, in most cases, the parameter name is always “connection”, so the Validate class provides an overload that provides that default name for you.)

    Option Strict On

     

    Imports System.Data

     

    Namespace Validation

     

       Public NotInheritable Class Validate

     

          Public Shared Function That(ByVal value As IDbConnection) _

                As ConnectionValidator

             Return New ConnectionValidator(value, "connection")

          End Function

     

          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

    Basically, Validate is just a factory; it spits out parameter validator objects, whose type is determined by the parameters that you pass. Once you get back an appropriately typed parameter validator, you’re free to invoke the methods that allow you to test the parameter for its suitability.

    The ConnectionValidator class below provides the methods to test a database connection’s suitability. (Its base class is shown immediately below it.)

    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 IsClosed()

             If Not Connection Is Nothing Then

                If (Connection.State And ConnectionState.Closed) = 0 Then

                   Throw New ArgumentException("Operation requires a closed connection.", Name)

                End If

             End If

          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 IsValid()

             IsNotNull()

             IsOpen()

          End Sub

     

          Private ReadOnly Property Connection() As IDbConnection

             Get

                Return DirectCast(InnerValue, IDbConnection)

             End Get

          End Property

       End Class

     

       Public MustInherit Class ParameterValidatorBase

     

          Protected Sub New(ByVal value As Object, ByVal name As String)

             m_value = value

             m_name = name

          End Sub

     

          Protected ReadOnly Property InnerValue() As Object

             Get

                Return m_value

             End Get

          End Property

     

          Public ReadOnly Property Name() As String

             Get

                Return m_name

             End Get

          End Property

     

          Private m_value As Object

          Private m_name As String

     

       End Class

    End Namespace

    The library contains strongly-typed validator classes for each of the intrinsic data types (String, Date, integer, long, short, boolean, and so on), transactions, and connections. It also contains a few additional utility types that I found useful, such as array validators (to check for bounds conditions, empty arrays, and null arrays) and collection validators.

    In any event, a sample chunk of validation code looks like this:

       Public Sub New(ByVal siteId As Integer, _

          ByVal startDate As Date, _

          ByVal endDate As Date, _

          ByVal offenderId As Integer, _

          ByVal logonName As String, _

          ByVal connection As SqlConnection)

     

          Validate.That(siteId, "siteId").IsPositive()

          Validate.That(startDate, "startDate").IsGreaterThanOrEqualTo(Constants.DefaultDate)

          Validate.That(endDate, "endDate").IsGreaterThanOrEqualTo(startDate)

          Validate.That(offenderId, "offenderId").IsGreaterThan(-1)

          Validate.That(offenderId, "offenderid").NotEqualTo(0)

          Validate.That(logonName,_ "logonName").IsNotNull()

          Validate.That(connection).IsValid()

     

          ...

     

       End Sub

    Which, in my opinion, is far more readable than the following:

          If siteId <= 1 Then

             Throw New ArgumentOutOfRangeException("siteId")

          End If

          If startDate < Constants.DefaultDate Then

             Throw New ArgumentOutOfRangeException("startDate")

          End If

          If endDate < startDate Then

             Throw New ArgumentOutOfRangeException("enddate")

          End If

          If offenderId < -1 Then

             Throw New ArgumentOutOfRangeException("offenderId")

          End If

          If offenderId = 0 Then

             Throw New ArgumentException("offenderId may not be zero.")

          End If

          If logonName Is Nothing Then

             Throw New ArgumentNullException("logonName")

          End If

          If connection Is Nothing Then

             Throw New ArgumentNullException("connection")

          End If

          If (connection.State And ConnectionState.Open) = 0 Then

             Throw New ArgumentException("Operation requires an open connection.")

          End If

    Now, there are several issues which I have yet to resolve (but am actively working on):

    1.       If you want to perform several tests on the same object, you have to create several different parameter validator objects. The current design is wasteful this way. I’d like to find a way around that.

    2.       It’s not very extensible. The core type support is great; but what about new types? Having to manually add a new method for every new type I want to support is problematic, and completely contrary to object oriented principles. I need to do something interface based, and I haven’t quite wrapped my brain around it yet. (I do know that I want to avoid the penalties inherent in reflection at all costs. This thing is used way too much to incur those kinds of costs.)

    3.       It’s not localized. But we’ve already mentioned that.

    So there you have it. Tell me what you think.

  • 07-20-2007 2:04 PM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    Looks beautiful! I can see where you could have issues with it (the routines being mutually exclusive), and it appears to be too tightly coupled with the database code for my tastes.

    But it looks like a very consistent pattern and a clean design. I'll enjoy stealing it....

  • 07-21-2007 7:08 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    I decided to make it an open source project and let it stand or fall on its own. It will be a C# library, free to the masses. You'll be able to find it at www.nvalidate.org.

     

  • 07-24-2007 9:27 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    Awesome! The masses thank you!

  • 07-24-2007 11:05 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    Thanks to reader input, I actually resolved some of the exclusivity with the methods and got it down to something like this:

    Validate.That(aString, "aString").IsNotNull().IsNotEmpty()

    So you can chain tests together. I've also figured out some extensibility options, so I'm looking at that. And I'm going to be looking for testers and contributers. *hint* *hint*

     

  • 07-25-2007 8:17 AM In reply to

    Re: Fuzziness and Overzealousnes In Parameter Validation

    count me in!

Page 1 of 1 (9 items)
Seminars           www.Construx.com           Consulting