VB Forum / General 2 / November 2004

So many problems <sigh>. :-(

1) why potentially 3 message boxes for the validation routine?  Assuming
your user has even minimal intelligence, give them one message indicating
what the three requirements for successfully entering data are, eg (air
code).

Private Sub validation1()

  Flag = (opt18.Value Or _
            opt31.Value Or  _
            opt41.Value Or _
            opt55.Value Or _
            opt0.Value Or  _
            optOver2.Value Or  _
            optOver10.Value) And
            Len(Text1.Text) > 0
End Sub

That said, personally I don't believe a validation routine should raise an
error (msgbox), but simply return a value the subsequent code can use to
determine whether to proceed:

Private Function IsValidInput() as boolean

  IsValidInput = (opt18.Value Or _
                       opt31.Value Or  _
                       opt41.Value Or _
                       opt55.Value Or _
                       opt0.Value Or  _
                       optOver2.Value Or  _
                       optOver10.Value) And
                       Len(Text1.Text) > 0
End Sub

This removes the requirement to have a public variable to store the status
of the validation routine:

Private Sub cmdCalculate_Click()

  Dim AnnualSalary, TotalPay As Currency

   If IsValidInput() Then

          AnnualSalary = txtAnnualSalary.Text
          ... and so on

2)  As has been mentioned umpteen times before:

  Dim AnnualSalary, TotalPay As Currency

...  creates a variant for the AnnualSalary variable. If you want a variant,
define it as such, otherwise define it to a specific data type. Since all
your calculation muck with the value assigned to AnnualSalary, make it a
numeric data type Single ...

Dim AnnualSalary As Single, TotalPay As Currency

3) Look carefully at the cmdCalculate_Click routine. Where can you see
improvement? I see two places right off the bat. First, dump the multiple
Val calls and do it once only ...

          AnnualSalary = Val(txtAnnualSalary.Text)

Naturally this has no checks for non-numeric entry, but we'll skip over that
for now.

Next, you are essentially doing the same calculations repeatedly, with a
different divisor based on opt0, optOver2, and optOver10 (or so it seems).
Why not perform one check to set a variable to indicate the divisor?

  Dim divisor as long

   Select case True  'some don't like this; I do.
        case opt0.value :  divisor = 4 :
        case optOver2.value:  divisor = 2
        case optOver10.value:  divisor = 1
   end select

Now you only need to test one set of option buttons, with one block of code:

   If (opt18.Value) Or (opt18.Value) Then
     TotalPay = (AnnualSalary * 0.025) + (AnnualSalary / divisor)

   ElseIf (opt31.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.05) + (AnnualSalary / divisor)

   ElseIf (opt41.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.075) + (AnnualSalary / divisor)

   ElseIf (opt55.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.1) + (AnnualSalary / divisor)
End If

In fact, you could nuke half the code since it's the same and again, perform
the calculations only once ...

  Dim divisor as long
  Dim adjustment as Single

   Select case True  'some don't like this; I do.
        case opt0.value :  divisor = 4
        case optOver2.value:  divisor = 2
        case optOver10.value:  divisor = 1
   end select

  adjustment = Val(AnnualSalary) / divisor

   If (opt18.Value) Or (opt18.Value) Then
     TotalPay = (AnnualSalary * 0.025) + adjustment

   ElseIf (opt31.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.05) + adjustment

   ElseIf (opt41.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.075) + adjustment

   ElseIf (opt55.Value) Or (optOver2.Value) Or (optOver10.Value) Then
     TotalPay = (AnnualSalary * 0.1) + adjustment  End If
   end if

You get the drift. Further optimization (and more readable code) can be
attained by doing the same thing for the rates the annualsalary is increased
based on option 18,31,41, and 55.  With this done you should be able to get
to a one-liner with no If statements that might look like:

  totalpay = increasedAnnualSalary + adjustment

4)  NEVER use a hard-coded file number such as #1. VB provides a function
for this -- FreeFile. Use it.

   dim hfile as long

    hfile = freefile
    open whatever for input as #hfile
       .. etc

5) If the ID value you're interested in is unique, you can load the file
into a single string and use Instr() to determine if there is a matching
string in the file ...

   dim hfile as long
   dim buff as string

    hfile = freefile
    open whatever for input as #hfile
        buff = input$(LOF(hfile), hfile)

       if instr(1, buff, "yourIDhere", vbTextCompare) then
             debug.print "Yep, it's in here already"

             ...etc

But is it on a line by itself in the file? Or is it surrounded by some
delimiters in the file (like this <123> maybe)? Or is it just a number
embedded on a line along with (possibly) other numbers?

Rick - MVP