optimize code

[Jusan]

hi, I have written a cheque writing utility in vb.net and would like to
invite comments on the code. can this be done in a better / faster / shorter
way?
thnx


Public Class ChequeWritingUtility
Inherits System.Windows.Forms.Form

' converts numbers to words
#Region " Windows Form Designer generated code "
Public Sub New()
MyBase.New()
'This call is required by the Windows Form Designer.
InitializeComponent()
'Add any initialization after the InitializeComponent() call
End Sub
'Form overrides dispose to clean up the component list.
Protected Overloads Overrides Sub Dispose(ByVal disposing As Boolean)
If disposing Then
If Not (components Is Nothing) Then
components.Dispose()
End If
End If
MyBase.Dispose(disposing)
End Sub
'Required by the Windows Form Designer
Private components As System.ComponentModel.IContainer
'NOTE: The following procedure is required by the Windows Form Designer
'It can be modified using the Windows Form Designer.
'Do not modify it using the code editor.
Friend WithEvents outputLabel As System.Windows.Forms.Label
Friend WithEvents convertButton As System.Windows.Forms.Button
Friend WithEvents Label1 As System.Windows.Forms.Label
Friend WithEvents numberTextBox As System.Windows.Forms.TextBox
<System.Diagnostics.DebuggerStepThrough()> Private Sub InitializeComponent()
Me.outputLabel = New System.Windows.Forms.Label
Me.convertButton = New System.Windows.Forms.Button
Me.Label1 = New System.Windows.Forms.Label
Me.numberTextBox = New System.Windows.Forms.TextBox
Me.SuspendLayout()
'
'outputLabel
'
Me.outputLabel.Location = New System.Drawing.Point(32, 46)
Me.outputLabel.Name = "outputLabel"
Me.outputLabel.Size = New System.Drawing.Size(296, 58)
Me.outputLabel.TabIndex = 7
'
'convertButton
'
Me.convertButton.DialogResult = System.Windows.Forms.DialogResult.OK
Me.convertButton.Location = New System.Drawing.Point(248, 120)
Me.convertButton.Name = "convertButton"
Me.convertButton.Size = New System.Drawing.Size(80, 24)
Me.convertButton.TabIndex = 6
Me.convertButton.Text = "Convert"
'
'Label1
'
Me.Label1.Location = New System.Drawing.Point(24, 14)
Me.Label1.Name = "Label1"
Me.Label1.Size = New System.Drawing.Size(96, 16)
Me.Label1.TabIndex = 5
Me.Label1.Text = "Enter Number"
'
'numberTextBox
'
Me.numberTextBox.Location = New System.Drawing.Point(128, 14)
Me.numberTextBox.Name = "numberTextBox"
Me.numberTextBox.Size = New System.Drawing.Size(200, 20)
Me.numberTextBox.TabIndex = 4
Me.numberTextBox.Text = ""
'
'ChequeWritingUtility
'
Me.AutoScaleBaseSize = New System.Drawing.Size(5, 13)
Me.ClientSize = New System.Drawing.Size(336, 157)
Me.Controls.Add(Me.outputLabel)
Me.Controls.Add(Me.convertButton)
Me.Controls.Add(Me.Label1)
Me.Controls.Add(Me.numberTextBox)
Me.MaximizeBox = False
Me.MinimizeBox = False
Me.Name = "ChequeWritingUtility"
Me.Text = "ChequeWritingUtility"
Me.ResumeLayout(False)
End Sub
#End Region
'The approach I have chosen provides for extensibility for adding higher
denominations.
Private Sub OnConvertButtonClicked(ByVal sender As System.Object, ByVal e As
System.EventArgs) Handles convertButton.Click
Dim number As String = numberTextBox.Text.Trim()
Dim dollars As String
Dim words As String
Dim position(2) As String 'for adding higher denominations increase the
array size.
Dim cents As String
Dim cntr As Int32
Dim decimalPos As Int32
If (numberTextBox.Text.Trim() = "") Then
Return
End If
position(1) = " Thousand "
position(2) = " Million "
'can add higher denominations here. this allows for easy extensibility.
'currently the program can accomodate upto million dollars.
'determines if the amount has cent values.
decimalPos = InStr(number, ".", CompareMethod.Text)
If (decimalPos > 0) Then
cents = GetTens(Microsoft.VisualBasic.Left(Microsoft.VisualBasic.Mid(number,
decimalPos + 1) & "00", 2))
number = (Microsoft.VisualBasic.Left(number, decimalPos - 1))
cents = " and " & cents & " cents"
End If
'determines the dollar amounts.
If (number.Length >= 10) Then
outputLabel.Text = "Enter less than 10 digits"
Return
End If
Do While number <> ""
words = GetHundreds(Microsoft.VisualBasic.Right(number, 3))
If words <> "" Then dollars = words & position(cntr) & dollars
If (number.Length > 3) Then
number = Microsoft.VisualBasic.Left(number, Len(number) - 3)
Else
number = ""
End If
cntr = cntr + 1
Loop
dollars = dollars & " dollars"
outputLabel.Text = dollars + cents
End Sub

Private Function GetHundreds(ByVal number As String) As String
Dim result As String
'calculates the hundreds.
number = Microsoft.VisualBasic.Right("000" & number, 3)
If Mid(number, 1, 1) <> "0" Then
result = GetOnes(Mid(number, 1, 1)) & " Hundred And "
End If
If Mid(number, 2, 1) <> "0" Then
'calculates the tens.
result = result & GetTens(Mid(number, 2))
Else
'calculates the ones.
result = result & GetOnes(Mid(number, 3))
End If
Return result
End Function
Private Function GetTens(ByVal number As String) As String
Dim result As String
If (Microsoft.VisualBasic.Left(number, 1) = 1) Then
Select Case (number)
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
Select Case Microsoft.VisualBasic.Left(number, 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-"
End Select
result = result & GetOnes(Microsoft.VisualBasic.Right(number, 1))
End If
Return result
End Function
Private Function GetOnes(ByVal number As String) As String
Select Case (number)
Case 1 : GetOnes = "One"
Case 2 : GetOnes = "Two"
Case 3 : GetOnes = "Three"
Case 4 : GetOnes = "Four"
Case 5 : GetOnes = "Five"
Case 6 : GetOnes = "Six"
Case 7 : GetOnes = "Seven"
Case 8 : GetOnes = "Eight"
Case 9 : GetOnes = "Nine"
End Select
Return GetOnes
End Function
End Class

 

[Jay B. Harlow [MVP - Outlook]]

Jusan,
First I would add "Option Strict On" and "Option Explicit On" to the top of
your source file, and fix all the implicit conversions.

With your functions, you do not need to set the function name & return the
function name. I would do one or the other.

' This is bad
Function GetOnes() As String
GetOnes = "one"
Return GetOnes
End Function

' This is good
Function GetOnes() As String
GetOnes = "one"
End Function

' This is also good, I normally use this one.
Function GetOnes() As String
Return "one"
End Function

I would consider using a StringBuilder (possible passing it as parameter)
instead of using String contention. Ideally I would probably create a
separate class that converts the value, using a StringBuilder as a member
field.

Just for consistency I would use either + or & for concatenation, not both.

To clean up the code I would use an Import alias for Microsoft.VisualBasic

Imports VB = Microsoft.VisualBasic

Public Class ChequeWritingUtility

Then where I needed to use Left & Right I would use VB.Left & VB.Right.

Instead of using a Select Case in GetOnes, GetTens, and GetHundreds I would
consider using an array, either Static to the routine or Shared in the
class.

Hope this helps
Jay

 

[Cor Ligthert]

Hi Jusan,

In addition to Jay I make as well some extra comments, however did you
know that the routine you are making exist on MSDN see for that this

\\\
<http://support.microsoft.com/?scid=http://support.microsoft.com:80/support/
kb/articles/Q95/6/40.ASP>
///

My comments on your code (as said in addition to Jay).

When you want to exit a method set the exit as high as possible, the style
of first setting the declarations of the values is from the past.

In your code you use a Return in a Sub, I would use the Exit Sub because it
confuses. A Return implements for me a function with a return value wich is
explicit or implicit given.

For the rest I see no big structural errors (I am never using the
VisualBasic namespace methods for string handling so maybe I miss something,
however those additions above was what direct got my eye).

I hope this helps?

Cor