Digiguru.co.uk

I was doing the enjoyable task of removing warnings from the code base today. We had a bad period of being stung by bugs that could have been prevented if the developer had heeded the warnings. Visual studio gives you a nice way to see the warnings as errors to make them more urgent. Right Mouse On Project In Solution Explorer > Properties > Compile > Treat all warnings as errors I can across a lot of code pulling objects out of session variables. This is tricky because the session stores the variables as objects, so you'll get this kind of warning.

Dim shopIDList As List(Of Guid)
shopIDList = Session.Item(gplSessionIdent.ToString)

I looked at our code base and realised someone had made a helpful extension method on session

If Session.TryGetValue(gplSessionIdent.ToString, shopIDList) Then

Looking at the method body you can see that it still deals with object, so still outputs an error Session.TryGetValue(param as String, ByRef value as Object) Seemed similar to what I wanted but sadly casted the value back to an object, leaving me in the same position i was in before. I decided to write another extension.

  <Extension()>
    Public Function TryConvertValueOf(Of T)(ByVal sess As HttpSessionState, ByVal param As String, ByRef output As T)
        Dim obj As Object = sess(param)
        If TypeOf obj Is T Then
            output = DirectCast(obj, T)
            Return output IsNot Nothing
        End If
        Return False
    End Function

Happy I decided to show my co-workers. Thankfully they spotted a flaw in that structures would probably misbehave so enhanced it slightly.

  <Extension()>
    Public Function TryConvertValueOf(Of T As {Class})(ByVal sess As HttpSessionState, ByVal param As String, ByRef output As T)
        Dim obj As Object = sess(param)
        If TypeOf obj Is T Then
            output = DirectCast(obj, T)
            Return output IsNot Nothing
        End If
        Return False
    End Function

So I was about to apply the code when my co-worker stopped me. He said it was bad practice. He showed me the original TryGetValue code and that it actually throws errors itself.

 <Extension()>
    Public Function TryGetValue(session As System.Web.SessionState.HttpSessionState,
                                sessionItemName As String,
                                ByRef result As Object) As Boolean
        If session Is Nothing Then
            Throw New ArgumentNullException("Session", "Session is nothing.")
        End If
        If String.IsNullOrEmpty(sessionItemName) Then
            Throw New ArgumentException("SessionItemName is nothing or empty.", "SessionItemName")
        End If
        result = session(sessionItemName)
        Return result IsNot Nothing
    End Function

You can find the full listing here I said that by naming a function Try... the developer assumes that the application will block any errors happening while it runs that code. Although he agreed, he insisted that code in this scenario should throw an exception. We've argued about this before. He hates it when code masks errors, and would far rather throw an error so that the developer knows there is a bug, rather than returning false, meaning that any error is now simply a Boolean. It's a good point. I think back to a lot of times where I've had to debug code only to find that actually the error was being eaten up in a try catch and never given to me so I had a stack trace. On the other hand I am also aware of times when we've had code that seems to work fine in the unit tests and in the QA tests, but when it got to the production system and a nasty bug appeared because it code finally hit an edge case that the APIs could have swallowed but actually deliberately told the developer that it was invalid (with a very useful message telling them how to fix it, as well as a stack trace). It makes me think about what is preferred. There's an insightful development law: Postals Law

Be conservative in what you send, liberal in what you accept.

How does one interpret this? Is John Postal stating that you should allow any junk to be thrown into your function and you should output simply a true or false? Or is being conservative actually outputting a true if the value exists false if the value is empty and an error if the value is junk? I can certainly see uses for both, consider the following example: If Session.TryConvertValueOf(Of Guid)("cus", CustomerId) Then ' lookup customer and do something with itElseIf Session.TryConvertValueOf(Of Customer)("cus", Customer) Then ' do something directly with the customerElse' no customer, assume there's nothing else to do. Maybe tell the user End If Okay now consider the next developer adds a list of customers to the session object, but neglects to update the code above. Unless they remember to debug line by line they might not realise their list is now being forgotten, and happily push the code even though its faulty. To use the error throwing version you would have to write something pretty nasty to mask the error prone code.

 Try
    success = Session.TryGetValue("cus", customerID)
    If success Then
       customer = LookupCustomer(customerID)
    End If
 Catch ex As Exception
    success = Session.TryGetValue("cus", customer)
  End Try
  If success Then
    ' do something with customer
  Else
    ' no customer, assume there's nothing else to do. Maybe tell the user
  End If

I think this is cumbersome and has little advantage in terms of readability, but certainly prevents stupidity. In summary I think that if you name a function Try... then you are telling the consumer of your API that there will be no error thrown. Generally the session is tricky to work with and if you store multiple types in the same value you are already setting yourself up for failure no matter how nice you wrap up an API to mask the underlying problem. Perhaps using a document storage database is the best practice. What do you think?