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 vb.net 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.