6
\$\begingroup\$

No problem yet with the code.

Here is a function that can be saved as add in and used as UDF in Microsoft excel to convert a number to text. For example, 4556.45 to RUPEES Four Thousand Five Hundred Fifty Six & PAISE Forty Five (Defaults being set to Indian Rupee). But the function can be adopted to any currency through suitable parameters making it very flexible. Submitting for review. Kindly suggest any modifications required. Thank you.


Function Arguments

myNumber = Number to be converted to text


Optional NumberSystem = DEFAULT Value = 2, 1 for International (Thousand,Million,Billion), 2 for Indian (Thousand,Lakh,Crore), Default Value 1


Optional CurrencyConversion = DEFAULT Value = "YES", _ Yes to convert the number to currency, Default Value Yes.


Optional CurrSYMSingular = DEFAULT Value = "RUPEE", 'for example USD or US DOLLAR, INR or Indian Rupee for one unit of currency, Default Value Rupee


Optional CurrSYMPlural = DEFAULT Value = "RUPEES", for example USDs or US DOLLARs, INRs or Indian Rupees for multiple units of currency, Default Value Rupees


Optional FractionSize = DEFAULT Value = 100, _ for example 100 for one INR = 100 Paise, one USD = 100 Cents, Default value 100


Optional FracSYMSingular = DEFAULT Value = "PAISA", for example Cent for US DOLLAR, Paisa for Indian Rupee for one unit of currency fraction, Default Value Paisa


Optional FracSYMPlural = DEFAULT Value = "PAISE", for example Cents for US DOLLAR, Paise for Indian Rupee for multiple units of currency fraction, Default Value Paise


Optional TextStyle = DEFAULT Value = 1 1 for CurrencySYM and Amount, 2 for Amount and CurrencySYM, Default Value 1"


Main function and other private functions supporting it are as below.

Function TextCurrency(ByVal myNumber, Optional NumberSystem = 2, Optional CurrencyConversion = "YES", _
Optional CurrSYMSingular = "RUPEE", Optional CurrSYMPlural = "RUPEES", Optional FractionSize = 100, _
Optional FracSYMSingular = "PAISA", Optional FracSYMPlural = "PAISE", Optional TextStyle = 1)

'Refer to following webpage for fractional units and sizes of different currencies
'https://en.wikipedia.org/wiki/List_of_circulating_currencies
' Maximum fraction size is 1000 (eg. OMR).

Dim Temp, myNumberInt, myNumberFrac, DecimalPlace, Count, RUPEEs, PAISE

If Val(myNumber) <> 0 Then
    myNumber = Trim(Str(myNumber))
    DecimalPlace = InStr(myNumber, ".")
End If
    
If DecimalPlace > 0 Then
    myNumberInt = Trim(Left(myNumber, DecimalPlace - 1))
    myNumberFrac = Trim(Mid(myNumber, DecimalPlace + 1))
    If UCase(CurrencyConversion) = "YES" Then
    If FractionSize <= 1000 Then myNumberFrac = Left(myNumberFrac & "000", 3)
    If FractionSize <= 100 Then myNumberFrac = Left(myNumberFrac & "00", 2)
    If FractionSize <= 10 Then myNumberFrac = Left(myNumberFrac & "0", 1)
    End If
Else
myNumberInt = myNumber
End If

If NumberSystem = 2 Then
    If Val(myNumberFrac) <> 0 Then
    TextCurrency = NumberINTtoINDtext(myNumberInt) & " POINT " & NumberFRACtotext(myNumber)
    Else
    TextCurrency = NumberINTtoINDtext(myNumberInt)
    End If
Else
    If Val(myNumberFrac) <> 0 Then
    TextCurrency = NumberINTtotext(myNumberInt) & " POINT " & NumberFRACtotext(myNumber)
    Else
    TextCurrency = NumberINTtotext(myNumberInt)
    End If

End If
    
If UCase(CurrencyConversion) = "YES" Then

    If NumberSystem = 2 Then
        If Val(myNumberFrac) <> 0 Then
        RUPEEs = NumberINTtoINDtext(myNumberInt)
        PAISE = NumberINTtotext(myNumberFrac)
        Else
        RUPEEs = NumberINTtoINDtext(myNumberInt)
        End If
    Else
        If Val(myNumberFrac) <> 0 Then
        RUPEEs = NumberINTtotext(myNumberInt)
        PAISE = NumberINTtotext(myNumberFrac)
        Else
        RUPEEs = NumberINTtotext(myNumberInt)
        End If
    
    End If
    
    If Val(myNumber) = 0 Then TextCurrency = CurrSYMSingular & " " & "ZERO"

    If TextStyle = 1 Then
         Select Case RUPEEs
            Case ""
               RUPEEs = ""
            Case "One"
               RUPEEs = CurrSYMSingular & " One"
            Case Else
               RUPEEs = CurrSYMPlural & " " & RUPEEs
         End Select

         Select Case PAISE
            Case ""
               PAISE = ""
            Case "One"
               If RUPEEs = "" Then
               PAISE = FracSYMSingular & " One"
               Else
               PAISE = " & " & FracSYMSingular & " One"
               End If
            Case Else
               If RUPEEs = "" Then
               PAISE = FracSYMPlural & " " & PAISE
               Else
               PAISE = " & " & FracSYMPlural & " " & PAISE
               End If
         End Select
    Else

         Select Case RUPEEs
            Case ""
               RUPEEs = ""
            Case "One"
               RUPEEs = "One " & CurrSYMSingular
            Case Else
               RUPEEs = RUPEEs & " " & CurrSYMPlural
         End Select

         Select Case PAISE
            Case ""
               PAISE = ""
            Case "One"
               If RUPEEs = "" Then
               PAISE = "One " & FracSYMSingular
               Else
               PAISE = " & One " & FracSYMSingular
               End If
            Case Else
               If RUPEEs = "" Then
               PAISE = PAISE & " " & FracSYMPlural
               Else
               PAISE = " & " & PAISE & " " & FracSYMPlural
               End If
         End Select
    End If

TextCurrency = RUPEEs & PAISE
End If

End Function

'___________________________________________________________

Private Function ConvertHundreds(ByVal myNumber)
Dim Result As String

    ' Exit if there is nothing to convert.
    If Val(myNumber) = 0 Then Exit Function

    ' Append leading zeros to number.
    myNumber = Right("000" & myNumber, 3)
    
    'Debug.Print myNumber

    ' Do we have a hundreds place digit to convert?
    If Left(myNumber, 1) <> "0" Then
       Result = ConvertDigit(Left(myNumber, 1)) & " Hundred "
    End If

    ' Do we have a tens place digit to convert?
    If Mid(myNumber, 2, 1) <> "0" Then
       Result = Result & ConvertTens(Mid(myNumber, 2))
    Else
       ' If not, then convert the ones place digit.
       Result = Result & ConvertDigit(Mid(myNumber, 3))
    End If

ConvertHundreds = Trim(Result)
End Function

'___________________________________________________________

Private Function ConvertTens(ByVal MyTens)
Dim Result As String
    ' Is value between 10 and 19?
    If Val(Left(MyTens, 1)) = 1 Then
       Select Case Val(MyTens)
          Case 10: Result = "Ten"
          Case 11: Result = "Eleven"
          Case 12: Result = "Twelve"
          Case 13: Result = "Thirteen"
          Case 14: Result = "Fourteen"
          Case 15: Result = "Fifteen"
          Case 16: Result = "Sixteen"
          Case 17: Result = "Seventeen"
          Case 18: Result = "Eighteen"
          Case 19: Result = "Nineteen"
          Case Else
       End Select
    Else
       ' .. otherwise it's between 20 and 99.
       Select Case Val(Left(MyTens, 1))
          Case 2: Result = "Twenty "
          Case 3: Result = "Thirty "
          Case 4: Result = "Forty "
          Case 5: Result = "Fifty "
          Case 6: Result = "Sixty "
          Case 7: Result = "Seventy "
          Case 8: Result = "Eighty "
          Case 9: Result = "Ninety "
          Case Else
       End Select

       ' Convert ones place digit.
       Result = Result & ConvertDigit(Right(MyTens, 1))
    End If

ConvertTens = Result
End Function

'___________________________________________________________

Private Function ConvertDigit(ByVal MyDigit)
Select Case Val(MyDigit)
   Case 1: ConvertDigit = "One"
   Case 2: ConvertDigit = "Two"
   Case 3: ConvertDigit = "Three"
   Case 4: ConvertDigit = "Four"
   Case 5: ConvertDigit = "Five"
   Case 6: ConvertDigit = "Six"
   Case 7: ConvertDigit = "Seven"
   Case 8: ConvertDigit = "Eight"
   Case 9: ConvertDigit = "Nine"
   Case Else: ConvertDigit = ""
End Select
End Function

'___________________________________________________________

Private Function NumberINTtotext(ByVal myNumber)
If Len(myNumber) = 0 Or IsNumeric(myNumber) = False Then
NumberINTtotext = ""
Exit Function
End If

Dim Temp
Dim myNumberInt, myNumberInteger
Dim DecimalPlace, Count
ReDim Place(9) As String
     Place(2) = " Thousand "
     Place(3) = " Million "
     Place(4) = " Billion "
     Place(5) = " Trillion "

     ' Convert MyNumber to a string, trimming extra spaces.
     myNumber = Trim(Str(myNumber))
     ' Find decimal place.
     DecimalPlace = InStr(myNumber, ".")
     ' If we find decimal place...
    
    If DecimalPlace > 0 Then
        myNumberInt = Trim(Left(myNumber, DecimalPlace - 1))
        Else
        myNumberInt = Trim(myNumber)
    End If
    
    If Val(myNumberInt) <> 0 Then
     Count = 1
     Do While myNumberInt <> ""
        ' Convert last 3 digits of MyNumber to English GBP.
        Temp = ConvertHundreds(Right(myNumberInt, 3))
        If Temp <> "" Then myNumberInteger = Temp & Place(Count) & myNumberInteger
        If Len(myNumberInt) > 3 Then
           ' Remove last 3 converted digits from MyNumber.
           myNumberInt = Left(myNumberInt, Len(myNumberInt) - 3)
        Else
           myNumberInt = ""
        End If
        Count = Count + 1
     Loop
     Else
     myNumberInteger = "ZERO"
     End If

NumberINTtotext = myNumberInteger
If Val(myNumber) = 0 Then NumberINTtotext = "ZERO"
End Function

'___________________________________________________________

Private Function NumberINTtoINDtext(ByVal myNumber)

If Len(myNumber) = 0 Or Val(myNumber) = 0 Or IsNumeric(myNumber) = False Then
    NumberINTtoINDtext = ""
    Exit Function
End If

Dim Temp
Dim RUPEEs, PAISE
Dim DecimalPlace, Count
ReDim Place(9) As String
    Place(2) = " Thousand "
    Place(3) = " Lac "
    Place(4) = " Crore "
    'Place(5) = " Arawb "
    'Place(6) = " Kharawb "
    'Place(7) = " Neel "
         
    myNumber = Trim(Str(myNumber))
    DecimalPlace = InStr(myNumber, ".")
    If DecimalPlace > 0 Then
        myNumber = Trim(Left(myNumber, DecimalPlace - 1))
    End If

Do
Count = 2
    If Len(myNumber) > 0 Then
        Hundreds = ConvertHundreds(Right(myNumber, WorksheetFunction.Min(3, Len(myNumber))))
        RUPEEs = Hundreds & RUPEEs
        myNumber = Left(myNumber, Len(myNumber) - WorksheetFunction.Min(3, Len(myNumber)))
    End If
                        
    Do While Count < 4
        Temp = ConvertHundreds(Right(myNumber, WorksheetFunction.Min(2, Len(myNumber))))
        If Temp <> "" Then RUPEEs = Temp & Place(Count) & RUPEEs
        myNumber = Left(myNumber, Len(myNumber) - WorksheetFunction.Min(2, Len(myNumber)))
        Count = Count + 1
    Loop

    If Len(myNumber) <> 0 And Count = 4 Then RUPEEs = " Crore " & RUPEEs
Loop While Val(Len(myNumber)) > 0

NumberINTtoINDtext = RUPEEs
End Function

'___________________________________________________________

Private Function NumberFRACtotext(ByVal myNumber)
Dim Temp, myNumberFrac, myNumberFraction, DecimalPlace, Count

If Len(myNumber) = 0 Or IsNumeric(myNumber) = False Then
    NumberFRACtotext = ""
    Exit Function
End If

     ' Convert MyNumber to a string, trimming extra spaces.
     myNumber = Trim(Str(myNumber))
     ' Find decimal place.
     DecimalPlace = InStr(myNumber, ".")
     ' If we find decimal place...
    
    If DecimalPlace > 0 Then
        myNumberFrac = Trim(Mid(myNumber, DecimalPlace + 1))
        Else
        NumberFRACtotext = "ZERO"
        Exit Function
    End If
    
    Count = DecimalPlace + 1
    Temp = ""
    Do While Val(Mid(myNumber, Count, 1)) = 0
    Temp = Temp & "ZERO "
    Count = Count + 1
    Loop
    
    Do While Count <> Len(myNumber) + 1
    If Val(Mid(myNumber, Count, 1)) = 0 Then
    Temp = Temp & "ZERO "
    Else
    Temp = Temp & ConvertDigit(Val(Mid(myNumber, Count, 1))) & " "
    End If
    Count = Count + 1
    Loop
NumberFRACtotext = Temp
End Function

After installing the function as add-in and before using the function, run this procedure to see the parameter descriptions in the function dialogue box. Better to save such procedures in personal excel book and to make it run on excel start.

Sub AddUDFToCustomCategory()
Application.MacroOptions Macro:="TextCurrency", Description:="Converts number to text", _
ArgumentDescriptions:=Array("Number to be converted to text", _
"1 for International (Thousand,Million,Billion)," & vbCrLf & "2 for Indian (Thousand,Lakh,Crore)," & vbCrLf & "Default Value 2", _
"Yes to convert the number to currency, Else please enter No" & vbCrLf & "Make sure the number is rounded to best suit the fraction size of the desired currency" & vbCrLf & "Default Value Yes", _
"for example USD or US DOLLAR, INR or Indian Rupee for one unit of currency," & vbCrLf & "Default Value Rupee", _
"for example USDs or US DOLLARs, INRs or Indian Rupees for multiple units of currency," & vbCrLf & "Default Value Rupees", _
"for example 100" & vbCrLf & "for one INR = 100 Paise, one USD = 100 Cents, One OMR = 1000 Baizas" & vbCrLf & "Default value 100", _
"for example Cent for US DOLLAR, Paisa for Indian Rupee for one unit of currency fraction," & vbCrLf & "Default Value Paisa", _
"for example Cents for US DOLLAR, Paise for Indian Rupee for multiple units of currency fraction," & vbCrLf & "Default Value Paise", _
"1 for CurrencySYM and Amount, for eample USD One Hundred Fifty," & vbCrLf & "2 for Amount and CurrencySYM, for eample One Hundred Fifty USD" & vbCrLf & "Default Value 1")
End Sub
\$\endgroup\$
3
  • \$\begingroup\$ For numbers, one can also refer to Microsoft webpage \$\endgroup\$
    – Naresh
    Jun 22 at 6:40
  • 2
    \$\begingroup\$ I have rolled back your edits. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Heslacher
    Jun 22 at 6:41
  • \$\begingroup\$ @Heslacher .. Thank you; was not sure of that. \$\endgroup\$
    – Naresh
    Jun 22 at 6:58
6
\$\begingroup\$

While I understand it's not generally done, I'm adding a review of the slightly updated code which should not have been posted as an answer. There seem to be sufficiently few changes made that it's not that much different than the original.

  • Implicit Variants - There are a large number of variables that are Dimmed, but no type is defined. These are implicitly declared Variant by the runtime engine and can lead to a variety of issues, the least of which is slower execution, the worst of which is probably future programmers (including future-you) misusing a variable because the "compiler" doesn't warn you that you're putting a String where a Long should be (for example), leading to run-time errors.
    • One example: Function TextCurrency(ByVal myNumber, ... right there at the top of the code block
  • Declaring multiple values in one Dim statement. While there's nothing inherently wrong with doing so, and some may consider it simply a matter of code style, it's generally frowned upon by "real" programmers, mostly because:
    • It's easy to miss that there are multiple variables declared on one line - most programmers assume one declaration per physical line and mentally stop reading after the first one
    • It's easy to forget to declare types for one or more of the variables in a line, for example, Dim X, Y, Z as Long is assumed by many to declare 3 variables of type Long, however, only Z is a Long, while X and Y are actually Variant.
    • It's preferred to declare variables as close to their initial use as possible and declaring multiple variables on one line tends to lead to a "wall of declarations" and a lot of scrolling to see what's actually going on
  • Declaring variables as Integer.
    • Internally, VBA works with 32-bit integers (i.e. Long) and it converts each 16-bit Integer to a 32-bit Long each time it needs to operate on one, so you may as well just declare them all as Long to begin with.
    • Frankly, there's no need to use Integer unless you're calling a function in some external DLL (like the WinAPI) that requires the use of a 16-bit integer.
    • You'll gain tiny bits of performance improvement with each Integer to Long change you make (by the runtime engine not having to do the conversion for you), and you'll remove (or at least put off) another opportunity for an overflow error.
  • Parameters are passed ByVal, yet are assigned a value
    • Once again Function TextCurrency(ByVal myNumber, ... is a culprit. The very first If statement contains myNumber = Trim(Str(myNumber)).
    • If you're expecting myNumber to be returned to the calling code different than how it's sent it, it should be ByRef so the changes can be reflected externally.
    • If you're not expecting to be returned, it makes more sense to use a local copy of it to be explicit and to write code that "Says what it does and does what it says".
  • Using "" to represent an empty string when there's a perfectly good built-in constant vbNullString to do so.
    • This is something of a matter of style, and it's certainly shorter to type ""
    • Using vbNullString makes it very explicitly clear what you mean, where "" could mean that you forgot to put something between those quotes.
  • Parameters are passed ByRef by default, but none are explicitly declared as such, and most appear to not need to be.
    • For example in Function TextCurrency(ByVal myNumber, Optional NumberSystem As Byte = 2, ... the 2nd parameter NumberSystem is passed ByRef but is never assigned to and could (and should) be passed ByVal to make it abundantly clear that this is the case
    • Function TextCurrency(ByVal myNumber, Optional ByVal NumberSystem As Byte = 2, ...
  • TextCurrency implicitly returns a Variant. You did remember to declare a return type for all your other functions. It should have As String at the end of the signature to help "Say what you mean and mean what you say".

The above are all issues that RubberDuck's Code Inspections caught (but not all the issues it found). The built in QuickFixes will give you one or more options and actually fix the code for you.


These are other observations:

Variable Naming

  • Variable names are somewhat declared with Hungarian Notation, yet are inconsistent with their variable type
    • Dim myNumberInt As String made me think that this should hold an Int, yet it's declared as String.
    • Either the variable type is wrong or the variable name is poor.
    • Naming variables is hard!
    • Based on use, it appears that myNumberInt is the whole number portion of the value passed to the function. I'd suggest something like inputValueIntegerPart, or, maybe a simpler integerPortion. While that's a lot to type, the VBE does offer auto-complete by pressing <Ctrl>-<Space> which will either complete the word for you or offer up a list of everything starting with what you've typed so far, so you don't have to type the whole thing every time.
  • myNumber isn't particularly descriptive of what it holds.
    • Something along the lines of inputValue or originalCurrencyAmount or completeCurrencyAmount are more descriptive and help you remember, many lines later, what you're dealing with.
  • Capitalization consistency
    • You declare the "camelCase" myNumber, yet use "PascalCase" for NumberSystem, and "SHOUTCASE" for RUPEEs and PAISE.
    • Convention says that methods (Sub and Function) are "PascalCase", while variables are "camelCase" and constants are "SHOUTCASE".
    • Of course, there isn't much convention in VBA (other than "no convention at all"), but since you care enough to get your code reviewed, you probably won't be writing VBA forever, and you may want to consider training your brain now for other languages in the future.
    • You don't have to follow that convention by any means (especially if you're a programming shop of 1), but pick some convention and follow it. It will make your life much easier.
    • Do be aware, however, than VBA "helpfully" fixes casing for you, and that can lead to annoying situations later. For example, if you Dim value As Long, you will end up with ThisWorkbook.Cells("A1").Value being "helpfully" fixed to ThisWorkbook.Cells("A1").value (note the lower case "v"). So A) try not to use common method names as variables (even though the scope is different and you can get away with it), and B) If you ever do, a simple Dim Value will "fix" all the occurrences in your code, then you can delete the line.

Indentation

  • This is always tricky as often formatting is mangled/lost when pasting code into the text entry box.
  • While code indention doesn't matter one wit to the VBA compiler, it matters hugely to the poor soul who has to read it. This example is a particularly egregious one:
    If CurrencyConversion Then
    If FractionSize <= 1000 Then myNumberFrac = Left(myNumberFrac & "000", 3)
    If FractionSize <= 100 Then myNumberFrac = Left(myNumberFrac & "00", 2)
    If FractionSize <= 10 Then myNumberFrac = Left(myNumberFrac & "0", 1)
    End If
  • You have a multi-line If statement wrapping several single-line If statements. The lack of indention in the outer If/End If block makes it harder to mentally parse and requires a new reader (or future you) to slow down and take more time to understand what's going on. Our brains will naturally associate the End If with the last If statement, when in fact, it's actually associated with the first If statement.
    If CurrencyConversion Then
      If FractionSize <= 1000 Then myNumberFrac = Left(myNumberFrac & "000", 3)
      If FractionSize <= 100 Then myNumberFrac = Left(myNumberFrac & "00", 2)
      If FractionSize <= 10 Then myNumberFrac = Left(myNumberFrac & "0", 1)
    End If
  • Is functionally identical, but makes the code structure much more obvious.

Function size

  • You have some bits of code pulled out into their own functions, and that's good, but the main TextCurrency function is still rather large.
  • There are a number of "chunks" of code that could stand on their own to make the main function more readable, for example:
  If Val(myNumber) <> 0 Then
    myNumber = Trim(Str(myNumber))
    DecimalPlace = InStr(myNumber, ".")
  End If

Could become

DecimalPlace = FindDecimalPlace(myNumber)

followed later by the function declaration:

Private Function FindDecimalPlace(ByVal inValue As Double) As Long

  If Val(inValue) <> 0 Then
    Dim valueAsString As Double
    valueAsString = Trim(Str(inValue))
    FindDecimalPlace = InStr(valueAsString, ".")
  End If

End Function
  • This replaces five lines of code in the main function with only one line, making the main function more readable and it's very explicit that this line is going to FindDecimalPlace and assign it to a variable to use later.

  • Another example would be extracting this into its own function:

If DecimalPlace > 0 Then
    myNumberInt = Trim(Left(myNumber, DecimalPlace - 1))
    myNumberFrac = Trim(Mid(myNumber, DecimalPlace + 1))
    If CurrencyConversion Then
    If FractionSize <= 1000 Then myNumberFrac = Left(myNumberFrac & "000", 3)
    If FractionSize <= 100 Then myNumberFrac = Left(myNumberFrac & "00", 2)
    If FractionSize <= 10 Then myNumberFrac = Left(myNumberFrac & "0", 1)
    End If
Else
myNumberInt = myNumber
End If
  • As I was reviewing, looking to see how to refactor this, I ended up with the following:
  Dim RUPEEs As String, PAISE As String
  'NOTE: I removed the `decimalPlace` variable declaration from above. 
  'because there are multiple declarations on this line, I had to edit the line instead of just deleting it

  'decimalPlace = FindDecimalPlace(myNumber)
  'NOTE: This line is being removed. I'm commenting it here to make it obvious, but it should be removed once code is proved to still work correctly, don't leave it behind, that doesn't help readability
    
  myNumberFrac = SetFractionSize(myNumber, myNumberInt, myNumberFrac)

  'the remainder of the TextCurrency function is here, followed later by

Private Function SetFractionSize(ByVal originalCurrencyAmount As Double, ByVal currencyConversion As Boolean, ByVal fractionSize As Long, _
                                 ByRef outIntegerPortion As String, ByRef outDecimalPortion As String) As String

  Dim decimalPlaceLocation As Long
  decimalPlaceLocation = FindDecimalPlace(originalCurrencyAmount)
  
  If decimalPlaceLocation > 0 Then
    outIntegerPortion = Trim(Left(originalCurrencyAmount, decimalPlaceLocation - 1))
    outDecimalPortion = Trim(Mid(originalCurrencyAmount, decimalPlaceLocation + 1))
    If currencyConversion Then
      If fractionSize <= 1000 Then SetFractionSize = Left(outDecimalPortion & "000", 3)
      If fractionSize <= 100 Then SetFractionSize = Left(outDecimalPortion & "00", 2)
      If fractionSize <= 10 Then SetFractionSize = Left(outDecimalPortion & "0", 1)
    End If
  Else
    outIntegerPortion = originalCurrencyAmount
  End If

End Function
  • Pay attention to the comments in the code block they explain a few of the things done and why

  • Also, note that the call to FindDecimalPlace was moved into this function since the original DecimalPlace variable is not used anywhere else in TextCurrency, therefore, it doesn't need to exist in TextCurrency, only in this function where it's actually used.

  • Also, all the ByVal parameters are listed first, followed by the ByRef parameters

    • While there's no convention that I'm aware of that encourages this, I did this to help mentally separate them
    • Also, the ByRef parameters use Systems Hungariation Notation to indicate that they are expected to be set in the function and return a value to be used "on the outside".
  • This changes the beginning of the TextCurrency function from a bulky:

Dim DecimalPlace As Integer, RUPEEs As String, PAISE As String

If Val(myNumber) <> 0 Then
    myNumber = Trim(Str(myNumber))
    DecimalPlace = InStr(myNumber, ".")
End If
    
If DecimalPlace > 0 Then
    myNumberInt = Trim(Left(myNumber, DecimalPlace - 1))
    myNumberFrac = Trim(Mid(myNumber, DecimalPlace + 1))
    If CurrencyConversion Then
    If FractionSize <= 1000 Then myNumberFrac = Left(myNumberFrac & "000", 3)
    If FractionSize <= 100 Then myNumberFrac = Left(myNumberFrac & "00", 2)
    If FractionSize <= 10 Then myNumberFrac = Left(myNumberFrac & "0", 1)
    End If
Else
myNumberInt = myNumber
End If

To a much more svelte and readable:

  Dim RUPEEs As String, PAISE As String

  myNumberFrac = SetFractionSize(myNumber, currencyConversion, fractionSize, myNumberInt, myNumberFrac)
  • There are many more opportunities for refactoring, I've only shown the first two obvious ones, and hope that the reasoning for them will help you find others.

Keep it DRY

  • DRY: Don't Repeat Yourself

    • If you discover you're writing the same line(s) of code over and over (or even just twice), refactor those lines into a function and call the function.
    • It makes the code more readable and more maintainable
  • You repeat this line in at least 3 locations:

    DecimalPlace = InStr(myNumber, ".")
    
  • Each of those can now be replaced with:

    DecimalPlace = FindDecimalPlace(myNumber) 'or whatever variable is appropriate to pass in
    
    • If you ever need to change the way you find the decimal point, or you find a bug, now you only need to fix it in the one function instead of tracking down everywhere the old version is and fixing it multiple times, most likely missing at least one of them.
    • Maybe you want to internationalize it and allow for a , as the decimal separator instead of the US (and, I am a bit surprised to find out, also Indian) . as the decimal separator - again, you only change it in one place
  • You also repeat this line in a number of places

    myNumber = Trim(Str(myNumber))
    
    • Remember above where it's also flagged as being passed ByVal, yet is assigned a value.
    • This is a good place to declare a new variable with a scope internal to TextCurrency and assign it one time, then assign that new variable to the return value of your newly written function.
    • You'll only use myNumber and never assign anything to it, plus you can skip all the other times you're Trim()ing it because you know it's already been cleaned up.

Code Separators

  • You have
'______________________________________________

Before every one of your functions. I fully understand why, however... Back in the "Tools", "Options" menu, check the "Procedure Separator" box and watch magic happen! :) enter image description here


There are a variety of other things you could do, too, but these are the big stand-outs that will make your code easier to read, easier to follow, easier to maintain in the future (for other programmers and future-you) and, much more importantly, less bug prone for all those reasons.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Wow, what an behemoth of an answer! +1 \$\endgroup\$ Jun 22 at 16:04
  • 1
    \$\begingroup\$ You've hung out here, @N3buchadnezzar, this one is short compared to some... \$\endgroup\$
    – FreeMan
    Jun 22 at 16:33
  • \$\begingroup\$ Can I upvote twice on this answer? :) Accepting this answer as I find it more specific towards code improvement. Really appreciate time and efforts you have given. Thank you. It's great to learn here. Also, thought of removing my answer as someone upvoted it,. But your this answer is referring to it. \$\endgroup\$
    – Naresh
    Jun 23 at 5:28
9
\$\begingroup\$

After a very quick glance, at an absolute minimum, you should declare a specific Type for all your parameters and variables, and ensure that they're the appropriate type.

For example, CurrencyConversion is a Variant, but it should be a Boolean, since that's how you're using it. The Boolean type is defined to handle simple Yes/No or True/False data types and to make dealing with them much easier. It would allow you to change:

If UCase(CurrencyConversion) = "YES" Then

to a much more simple, clear and readable:

If CurrencyConversion Then

You would do this by changing this part of the function signature from:

Optional CurrencyConversion = "YES",

to:

Optional CurrencyConversion As Boolean = True,

You'll end up with much more readable code that's easier to use, less prone to odd run-time errors due to accidental implicit conversions or the accidental misspelling of CurencyConversion (which will cause the VBA run-time to create a whole new variant variable initialized to... something), and, as a bonus, it'll run a bit faster since the VBA run-time engine won't have to figure out what type of data it's working with each and every single time it's referencing any one of your variables.

The very best way to ensure all your variables are declared is to use

Option Explicit

At the top of each and every code module. In fact, the VBE makes it easy to do this - simply go to the Tools menu, select Options, then ensure the Require Variable Declaration option is checked. enter image description here

Note that this will add Option Explicit to all new modules you create, but it won't go back and add it to existing ones - you'll have to do that by hand. What it will do, though, is cause a compile time error any time you attempt to use an undeclared variable, and that will catch typos like using CurencyConvert when you meant to use CurrencyConvert.

You can use the RubberDuck* add-in for the VBE to catch these undeclared variables and offer you a simple way to fix them. It will also catch that you're not using Option Explicit and offer to insert it into all your code modules for you. This is just one of the dozens of static code analysis tools it offers, as well as many, many other features that help drag the VBE much closer to the standards of other, more modern code editors.

*NOTE: RubberDuck is a free, open source project hosted on GitHub, I'm a big fan of RubberDuck and use it regularly in my daily work. I've also made a few contributions to the project.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you. Edited to code as per your suggestion. Also, noticed and removed some redundant variables. Sure will get RubberDuck, new learning for me :) \$\endgroup\$
    – Naresh
    Jun 22 at 6:34

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.