10
\$\begingroup\$

I have been using the TryXXX and GetXXX pattern lately to give clients a choice whether to trap an exception or to read a boolean. An example of this concept would be System.Integer.TryParse and System.Integer.Parse.

It is my understanding that you should get two benefits from this:

  1. using an if statement in calling code instead of try-catch
  2. a performance benefit from not causing an exception.

In this class, I am attempting to do this, but I think I am getting only benefit 1, not benefit 2. I am drawing a blank as how to not throw any exception without code duplication.

       Public Function ConvertFractionToDecimal(ByVal fraction As String) As Decimal

        If String.IsNullOrWhiteSpace(fraction) Then Throw BuildBadFractionException(fraction)

        'will accept fractions of the form:
        'X-Y/Z
        'X Y/Z
        'Y/Z
        'will not accept negative signs in numerator, denominator, or whole number

        Dim whole As String
        Dim numen As String
        Dim denom As String

        Dim loc1 As Integer
        Dim loc2 As Integer

        fraction = fraction.Trim
        If fraction Like "*?-*?/?*" Then
            loc1 = InStr(fraction, "-")
            loc2 = InStr(fraction, "/")
        ElseIf fraction Like "*? *?/?*" Then
            loc1 = InStr(fraction, " ")
            loc2 = InStr(fraction, "/")
        ElseIf fraction Like "*?/?*" Then
            loc2 = InStr(fraction, "/")
        Else
            Throw BuildBadFractionException(fraction)
        End If

        If loc1 > 0 Then
            whole = Mid(fraction, 1, loc1 - 1).Trim
        Else
            whole = "0"
        End If
        numen = Mid(fraction, loc1 + 1, loc2 - loc1 - 1).Trim
        denom = Mid(fraction, loc2 + 1).Trim

        Dim Uwhole As UInt32
        Dim Unumen As UInt32
        Dim Udenom As UInt32
        If Not UInt32.TryParse(whole, Uwhole) Then Throw BuildBadFractionException(fraction)
        If Not UInt32.TryParse(numen, Unumen) Then Throw BuildBadFractionException(fraction)
        If Not UInt32.TryParse(denom, Udenom) Then Throw BuildBadFractionException(fraction)

        Return CDec(Uwhole + Unumen / Udenom)

    End Function

    Public Function TryConvertFractionToDecimal(ByVal fraction As String, ByRef result As Decimal) As Boolean
        Try
            result = ConvertFractionToDecimal(fraction)
            Return True
        Catch ex As Exception
            result = 0
            Return False
        End Try
    End Function

    Private Function BuildBadFractionException(ByVal fraction As String) As InvalidOperationException
        Return New InvalidOperationException(String.Format("'{0}' is not a recognisable fraction.", fraction))
    End Function
\$\endgroup\$
6
\$\begingroup\$

In your case, you could simply have Try... implement all the logic:

Public Function TryConvertFractionToDecimal(ByVal fraction As String, ByRef result As Decimal) As Boolean

    ' Put your big block of code here, just replace
    ' Throw BuildBadFractionException(fraction) with
    ' Return False.
    ...

End Function

Public Function ConvertFractionToDecimal(ByVal fraction As String) As Decimal
    Dim result As Decimal
    If TryConvertFractionToDecimal(fraction, result) Then
        Return result
    Else
        Throw BuildBadFractionException(fraction)
    End If
End Function

This should satisfy both of your goals:

  1. No exception is thrown unless absolutely required.
  2. No code duplication.

Note that this only works because you throw exactly one type of exception. If you change that (for example, it might be more consistent with the BCL to throw ArgumentNullException, ArgumentException, FormatException and OverflowException), you will need to create two methods that don't call each other and extract common code into private helper methods to avoid code duplication.

\$\endgroup\$
  • \$\begingroup\$ I think this is a clean solution. I don't really see what is gained by multiple exception types in this case. Thanks for the suggestion, kinda obvious now, but it wasn't this afternoon . . . \$\endgroup\$ – Mike Sep 11 '14 at 1:01
  • 1
    \$\begingroup\$ By throwing multiple types of exception your client gains the ability to figure out what went wrong with the conversion. ArgumentNullException is clearly a programming error. FormatException is when something is not as it's expected (- sign in your case, or invalid UInt32). By differentiating just these two I (as caller) can write two separate catches and behave differently: ANE -> show bug report dialog, FE -> ask the user nicely to correct the error. It was always worth the effort to write a nice clean error message instead of being lazy and saying "It doesn't work! Fix it!" \$\endgroup\$ – TWiStErRob Sep 11 '14 at 19:22
8
\$\begingroup\$

I was going to edit my first answer, but as I wrote the example code I wanted to show, and refactored, and refactored again, ...the result went so far away from the original code that I thought it warranted a separate answer.


So I copied your code into Visual Studio, and started by implementing the changes I suggested in my other answer. Quickly though, I felt the need to extract a FractionInfo type, a Structure that would hold the Whole, Numerator and Denominator parts of a fraction, so that I could write the regex part into a separate function that would return an instance of this FractionInfo type.

Being a Structure, I wanted to make that type immutable, so I implemented an Empty property that returned a Shared default instance... and then I tried doing If result = FractionInfo.Empty and noticed the = operator needed to be implemented - I'm lazy, so instead I just implemented Equals, and since when you override Equals you also need to override GetHashCode, I implemented GetHashCode as well.

So the ConvertFractionToDecimal function started like this:

Dim info As FractionInfo = GetFractionInfo(value)
If info.Equals(FractionInfo.Empty) Then Throw BuildBadFractionException(value)

And then I thought "great, now I can get rid of pretty much the whole rest of the code!" ...and then it struck me: you're not showing where your Convert.../TryConvert... methods are written, but I'm assuming they're not written in a Structure called Fraction.

And to follow the single responsibility principle, the only logical place to put such conversion methods would be in a value type called Fraction.

I suggest you take a look at this question and answers (disclaimer: it's one of my questions on this site) - it's C#, but the basic idea is essentially the same.

After much refactoring, this is what I ended up with - I'm not fully satisfied with it because it will throw a FormatException whenever parsing fails, even when one would expect an OverflowException. It does throw an ArgumentNullException when you give it Nothing, but the produced stack trace might be a little surprising.

Private Shared Function TryParseMatchGroups(ByVal groups As Match, ByRef result As Fraction) As Boolean

    Dim success As Boolean
    Dim wholePart As String = groups.Groups("whole").Value
    Dim numeratorPart As String = groups.Groups("numerator").Value
    Dim denominatorPart As String = groups.Groups("denominator").Value

    Dim whole As Integer
    Dim numerator As Integer
    Dim denominator As Integer

    success = Integer.TryParse(IIf(String.IsNullOrEmpty(wholePart), "0", wholePart), whole) _
        And Integer.TryParse(IIf(String.IsNullOrEmpty(numeratorPart), "0", numeratorPart), numerator) _
        And Integer.TryParse(IIf(String.IsNullOrEmpty(denominatorPart), "0", denominatorPart), denominator)

    If success Then
        result = New Fraction(whole, numerator, denominator)
    Else
        result = Fraction.Empty
    End If

    Return success

End Function

Public Shared Function Parse(ByVal value As String) As Fraction

    Dim result As Fraction
    Dim match As Match = regexp.Match(value)

    If match.Success And TryParseMatchGroups(match, result) Then
        Return result
    Else
        Throw New FormatException()
    End If

End Function

Public Shared Function TryParse(ByVal value As String, ByRef result As Fraction) As Boolean

    If value Is Nothing Then Return False

    Dim match As Match = regexp.Match(value)
    Return match.Success And TryParseMatchGroups(match, result)

End Function

These functions are Shared, exactly like Decimal.Parse and Decimal.TryParse are. Here's the rest of the type:

Public Structure Fraction

    Private Const pattern As String = "^((?<whole>[0-9]+)?\s+?)?(?<numerator>[0-9]+)\s?/\s?(?<denominator>[0-9]+)\s*?$"
    Private Shared ReadOnly regexp As Regex = New Regex(pattern)
    Private Shared ReadOnly emptyValue As Fraction = New Fraction()

    Private ReadOnly WholePart As Integer
    Private ReadOnly NumeratorPart As Integer
    Private ReadOnly DenominatorPart As Integer

    Public Sub New(ByVal whole As Integer, ByVal numerator As Integer, ByVal denominator As Integer)
        WholePart = whole
        NumeratorPart = numerator
        DenominatorPart = denominator
    End Sub

    Public ReadOnly Property Whole() As Integer
        Get
            Return WholePart
        End Get
    End Property

    Public ReadOnly Property Numerator As Integer
        Get
            Return NumeratorPart
        End Get
    End Property

    Public ReadOnly Property Denominator As Integer
        Get
            Return DenominatorPart
        End Get
    End Property

    Public Shared ReadOnly Property Empty As Fraction
        Get
            Return emptyValue
        End Get
    End Property

    Public Function ToDecimal() As Decimal
        Return Whole + CDec(Numerator) / CDec(Denominator)
    End Function

    Public Overrides Function Equals(obj As Object) As Boolean
        Dim other As Fraction = DirectCast(obj, Fraction) 'let it blow up on InvalidCastException
        Return other.Whole = Me.Whole _
            And other.Denominator = Me.Denominator _
            And other.Numerator = Me.Numerator
    End Function

    Public Overrides Function GetHashCode() As Integer
        'ok this is a bit like cheating.. but so much easier than implementing the real thing
        Return Tuple.Create(Me.Whole, Me.Numerator, Me.Denominator).GetHashCode()
    End Function

    Public Overrides Function ToString() As String
        If Me.Whole <> 0 Then
            Return String.Format("{0} {1}/{2}", Me.Whole, Me.Numerator, Me.Denominator)
        Else
            Return String.Format("{0}/{1}", Me.Numerator, Me.Denominator)
        End If
    End Function

You'll notice this implementation doesn't support negative fractions either - I'll leave that for you to explore and implement yourself. Note that isn't my primary language, so there might be stylistic errors in here.

If you looked at my A fraction of the code post, you've noticed there's many, many members missing here, mostly operators; you'll probably want a Fraction to at least support the = and <> operators, but the Equals implementation might be good enough as well. To make the type more usable you'll want to add MinValue and MaxValue, and perhaps a Zero value (0/1) and a Simplify() method. And of course you'll want to support negative fractions.

Basically, what this answer is suggesting that no answer has suggested yet, is to implement a [full-blown?] Fraction type, and to encapsulate your parsing logic in there: putting it anywhere else will break SRP.

\$\endgroup\$
  • 5
    \$\begingroup\$ Wow. I was just making a shared library function to feed in a fraction string that I would encounter in AutoCAD drawings and text files and to give me back a decimal. This is far more robust and is the kind of type that I could use for years. Since I do in-house software for a manufacturing company, I suspect your fraction type will find its way into numerous different projects. \$\endgroup\$ – Mike Sep 11 '14 at 14:25
7
\$\begingroup\$

If I understand what you're doing correctly, then it looks like "-1/2" wouldn't return the correct value, and yet a negative numerator and/or denominator is perfectly ok in any fraction.

If Not UInt32.TryParse(whole, Uwhole) Then Throw BuildBadFractionException(fraction)
If Not UInt32.TryParse(numen, Unumen) Then Throw BuildBadFractionException(fraction)
If Not UInt32.TryParse(denom, Udenom) Then Throw BuildBadFractionException(fraction)

This completely defeats the purpose of using TryParse - I would throw a custom exception, yes, but making the inner exception the exception thrown by UInt32.Parse... and that quickly becomes an ugly try..catch mess.

I think a lot of your issues could be solved with a little Regular Expression. I believe something like this would make your code much easier to write.. and read:

Dim fractionPattern As String = "(?<numerator>\-?\d+)\s?\/\s?(?<denominator>\-?\d+)"

If you don't know Regular Expressions, this is how to read it:

  • (?<numerator>...) is a named capture group called "numerator"
  • \-? will capture a literal - that may or may not be there (the escape backslash might not be needed though)
  • \d+ will capture one or more digits
  • \s? will capture a whitespace that may or may not be there
  • \/ will capture a literal / (again, the escape backslash might not be needed)
  • (?<denominator>...) is another named capture group called "denominator"

And then you use the System.Text.RegularExpressions.Regex class to match the passed string parameter against the regex pattern.

If there's a match, your numerator and denominator are in the Match.Groups collection; you can safely parse them into Integer types, because anything else wouldn't have matched the regex anyway - that's when you can throw your custom exception.

I'd make both Convert... and TryConvert... methods use the same private function for that part, so no code duplication going on here; the only difference is that you can throw a custom exception if the input string doesn't match the regex pattern.

The TryConvert... method doesn't have to catch any exception that way, except you'll have to watch out for overflows.. so I'd still write it in a try...catch block, just for that reason.

Your code also needs to verify whether you're about to divide by zero, and avoid trying to do that at all - don't let an exception that can be avoided be thrown!

\$\endgroup\$
  • \$\begingroup\$ No time to include example code, but will edit when I get home, promise :) \$\endgroup\$ – Mathieu Guindon Sep 10 '14 at 18:01
  • \$\begingroup\$ Good points that I am not accounting for overflow or divide by zero. Negatives were not in my intent, but it would be good to handle correctly for future use. Regular expressions are one of those things I have been meaning to learn, but haven't yet because I have been getting by with "like" which is like watered down Regex. Thanks for offering to write an example. \$\endgroup\$ – Mike Sep 11 '14 at 0:59
  • \$\begingroup\$ @Mike I started editing... but it would have been a complete rewrite, so I posted another answer ;) \$\endgroup\$ – Mathieu Guindon Sep 11 '14 at 3:59
5
\$\begingroup\$

I wish I knew how to answer your question, but I don't see a way to do this without causing exceptions to begin with. Unfortunately, digging into the source code for TryParseInt32 didn't really help. That said, there's still some things to say about the code.

  • I usually discourage the use of xml documentation, but I think this is a good use case for it. It would be helpful to get this information in a tool tip.

    'will accept fractions of the form:
    'X-Y/Z
    'X Y/Z
    'Y/Z
    'will not accept negative signs in numerator, denominator, or whole number
    
  • Why shorten the variable names like this? Go ahead and spell it out. There's no reason not to; intellisense will save you the keystrokes.

    Dim numen As String
    Dim denom As String
    
  • These are also not good names. I guess they mean local1 and local2? That's no better than temp2 and temp2 in my book.

    Dim loc1 As Integer
    Dim loc2 As Integer
    

    Perhaps wholePartIndex and fractionalPartIndex would be better.

  • This smells of Hungarian notation. What happens if you need to change the type later? The name will no longer make sense. It encodes information about the value type into the name. That argument probably doesn't apply here, but is generally true. Avoid doing this.

    Dim Uwhole As UInt32
    Dim Unumen As UInt32
    Dim Udenom As UInt32
    
  • What's the point of having a BuildBadFractionException if your TryConvertFractionToDecimal function catches all Exceptions? You're swallowing exceptions that you shouldn't be. You should only be catching BuildBadFractionExceptions. Others should be allowed to bubble up.

\$\endgroup\$
  • \$\begingroup\$ "loc1" was location (in the string), but I guess it could be better named. The reason I made BuildBadFractionException was so that callers that use the non-Try version would get a consistent exception without me writing String.Format(message) a bunch of times in the main function. \$\endgroup\$ – Mike Sep 10 '14 at 17:35
  • 1
    \$\begingroup\$ Your link was useful though. The Microsoft solution seems to be "just duplicate it and quit worrying". See StringToNumber and TryStringToNumber in referencesource.microsoft.com/#mscorlib/system/number.cs It is the same logic in two places, one passing a boolean for failure, one throwing exception. I, for one, do not like the duplication, but it gives one perspective. \$\endgroup\$ – Mike Sep 10 '14 at 17:47
2
\$\begingroup\$

IMHO, the best pattern would have been one which Microsoft explicitly discourages: having the the primary logic be in an overload which accepts a parameter indicating what failures should be considered "expected" [the more commonly-used TryParse and Parse overloads would chain to that]. If TryParseInt32 had included an overload with a ThrowOnFailure parameter, then ParseInt32 and the normal overload of TryParseInt32 could simply chain to that. You could then have a TryConvertFractionToDecimal overload with a ThrowOnFailure parameter which would get passed to the corresponding overload of TryParseInt32. Bingo--no code duplication.

Since Microsoft didn't do that, your best bet might be to write your own ParseInt32 method which includes a ThrowOnFailure parameter and chains to either ParseInt32 or TryParseInt32 as appropriate. In that case, while the extra layer of wrapping on ParseInt would be a little annoying, the logic for TryConvertFractionToDecimal would only need to be implemented once.

Incidentally, while ThrowOnFailure could be a bool, it might be better to define an enumerated type; the meaning of x=TryParse(someString, FailureMode.ThrowOnFailure); would be much clearer than x=TryParse(someString, true);.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.