Are 'TryAction()' methods evil?

Question

I'm trying to improve FxCop compliance in my code (for the first time ever), but I'm a little stuck on an ethical issue. I have a method GetText() that returns a string from a remote server, but can throw an exception under certain circumstances. Which is why I also have a method TryGetText(ByRef text As String) that returns a boolean which indicates whether the call was successful. The return value is assigned to the text variable if true.

I thought this construction was perfectly acceptable, considering that even Microsoft does it (e.g. Integer.TryParse). FxCop is tut-tutting me though, dictating "Thou shalt not pass by reference!"

To circumvent this warning (and there are quite a few of them), I replaced the parameter with a StringBuilder. But despite now being compliant, I don't think it really improved my code in any way.

Before:

    Public Function TryGetText(ByRef text As String) As Boolean
        Dim command As New GetTextCommand(Me)
        Dim result As CommandResult = ProcessCommand(command, True)
        If result.CommandStatus <> Constants.Status.Failed Then
            text = result.Text
            Return True
        Else
            Return False
        End If
    End Function

After:

    Public Function TryGetText(builder As Text.StringBuilder) As Boolean
        Dim command As New GetTextCommand(Me)
        Dim result As CommandResult = ProcessCommand(command, True)
        If result.CommandStatus <> Constants.Status.Failed Then
            builder.Clear()
            builder.Length = result.Text.Length
            builder.Append(result.Text)
            Return True
        Else
            Return False
        End If
    End Function

Is this acceptable use of ByRef, or should I use the stringbuilder alternative? I'm not feeling very comfortable about suppressing this warning for every method where I use this construct. I don't feel like the stringbuilder variant improved code usability either.

Answer 1

Well, I think everybody hates too many byrefs. Two are already too many. Although in your case having many methods and they all follow the same pattern does not sound like a real issue. You have the option to not stick to Microsoft's pattern of Try* methods.

Instead of a Boolean, why not returning the string if successful or Nothing/Empty if it fails? Then you can test your TryGetText output with String.IsNullOrEmpty(resultText).

It's more code, indeed, but it does solve the warning (if that's really what you're after).

Answer 2

That's probably a good use of ByRef, so I'd be inclined to add an exception for that case. If you're using the code analysis feature in Visual Studio, you can simply add the following attribute to the method:

<System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1045:DoNotPassTypesByReference", MessageId = "1#")> _
Public Function TryGetText(ByRef text As String) As Boolean

Answer 3

The TryAction methods are quite useful when creating things like thread safe classes. It combines what would be two steps, test and action, into a single atomic operation. Microsoft makes use of it heavily in the Concurrent collection classes. For example http://msdn.microsoft.com/en-us/library/dd287191.aspx#Y0

So I think a blanket statement of "Thou shalt not pass by reference." should not be taken as gospel in all cases. There are legitimate uses. In your specific case, the ByRef seems much less clumsy than the StringBuilder version. But if a simple string GetText() that returns null/Nothing in the case you're currently returning false is equivalent, that would seem best.

Answer 4

Passing by reference makes things complicated when you try to use your method in functional style. You might consider using a nullable type or a tuple or your own Option type.

Since I am not that familiar with VB, here a C# example:

struct Option<T> {
   public bool ContainsElement { get; private set; }
   private T element;
   public T Element {
      get {
         if (!ContainsElement) throw new NoElementException ();
         return element;
      }
      set {
         element = value;
         ContainsElement = true;
      }
   }
   public T GetElementOrDefault (T defaultValue) {
      return ContainsElement ? element : defaultValue;
   }
}

Option<string> GetText () {
   ...
}