1

This code checks if a string contains a certain text to set other values to something.

I structured my code to contain a lot of if elseif statements which I have been told makes my code less efficient and slower.

I did begin a table to try to use VLookUp.

Picture of my data:

spreadsheet screenshot

Portion of my code.

If wsName = "TEST-3" Then
If rate_value < 50 Then
    vol = 95
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    MsgBox "Less than 50."
ElseIf rate_value = 50 Then
    vol = 98
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 49.8
    sweep_value_max = 50.2
ElseIf rate_value = 100 Then
    vol = 110
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 99.8
    sweep_value_max = 100.2
Else: rate_value = 200
    vol = 110
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 199.4
    sweep_value_max = 200.4
End If

ElseIf wsName = "TEST-8" Then
If rate_value < 50 Then
    vol = 98
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    MsgBox "Less than 50."
ElseIf rate_value = 50 Then
    vol = 98
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 49.8
    sweep_value_max = 50.2
ElseIf rate_value = 100 Then
    vol = 125
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 99.8
    sweep_value_max = 100.2
Else: rate_value = 200
    vol = 125
    Call updateSD(sysnum, vol_rowindex, max, vol)
    Call updateSD(sysnum, vol_rowindex_1, max, vol)
    sweep_value = 199.4
    sweep_value_max = 200.4
End If
End If

Call updateSD(sysnum, rate_rowindex, typ, rate_value)
Call updateSD(sysnum, rate_rowindex_1, typ, rate_value)
Call updateSD(sysnum, rate_rowindex_1, min, sweep_value)
Call updateSD(sysnum, rate_rowindex_1, max, sweep_value_max)

Sub updateSD(sysnum As String, rowindex As Double, columnindex As Long, Value As Double) 
    Worksheets(sysnum).Cells(rowindex, columnindex) = Value
    Worksheets(sysnum).Cells(rowindex, columnindex).Interior.Color = vbYellow
End Sub
double-beep
  • 5,031
  • 17
  • 33
  • 41
  • 1
    On a side note, the `Call` keyword is considered deprecated i.e. you could e.g. simply use `updateSD sysnum, vol_rowindex, max, vol`. Also, you could probably write the two Call-lines only twice, below each inner `End If`. – VBasic2008 Oct 21 '22 at 18:56
  • @VBasic2008 If i remove the `Call` keyword then I get an error saying "Expected: =" –  Oct 21 '22 at 19:04
  • @VBasic2008 It may be one time deprecated - but it in my opinion it is not any more, because of transferring code to vb.net it makes it much easier that the brackets are implemented. (And it is good for beginners not to fall for missing brackets) – Red Hare Oct 21 '22 at 19:05
  • @resu_student here we are - if you remove call you have to remove the brackets in your example. – Red Hare Oct 21 '22 at 19:07
  • @resu_student you can probably remove 2 `Call updateSD...` and put them once at the end of the if blocks, also you could use select case. – milo5m Oct 21 '22 at 19:12
  • @RedHare Ok, can you explain the purpose of removing the `Call` I am new to VBA –  Oct 21 '22 at 19:18
  • If your code is slow, it isn't due to the `If` statements, it's due to `updateSD`. Could you share its code? – VBasic2008 Oct 21 '22 at 19:18
  • @milo5m you mean so have it look like this at the end `Call updateSD(sysnum, sweeprate_rowindex, spectyp, sweeprate_value) Call updateSD(sysnum, sweeprate_rowindex_1, spectyp, sweeprate_value) Call updateSD(sysnum, sweeprate_rowindex_1, specmin, sweep_value) Call updateSD(sysnum, sweeprate_rowindex_1, specmax, sweep_value_max) Call updateSD(sysnum, snapdownvol_rowindex, specmax, snapdownvol) Call updateSD(sysnum, snapdownvol_rowindex_1, specmax, snapdownvol)` –  Oct 21 '22 at 19:19
  • Just before those 4 at the end... Also I hope that you don't use byref in those subs :) – milo5m Oct 21 '22 at 19:20
  • @VBasic2008 Just updated it with `updateSD` –  Oct 21 '22 at 19:20
  • @milo5m no I do not use `ByRef` –  Oct 21 '22 at 19:21
  • changing color and setting value * 6 uhm, and who knows what else before that :) Why don't you use conditional formatting? – milo5m Oct 21 '22 at 19:24
  • 1
    @RedHare re Call: [this is worth a read](https://stackoverflow.com/a/56874163) – chris neilsen Oct 21 '22 at 19:37
  • Although it is advisable to have many small procedures, you can't always successfully use them. `updateSD` writes and highlights a single cell. If you put it in a loop with thousands of cells, it will take some time. To speed things up, you want to write to and highlight the worksheet only once. If you could share the complete procedure for this part of writing and highlighting, and describe in detail what needs to be done, surely someone could provide an efficient solution. – VBasic2008 Oct 21 '22 at 19:44
  • @chrisneilsen No, it is not worth. Not for me. – Red Hare Oct 21 '22 at 19:48

1 Answers1

0

Maintenance and Readability

  • Not tested!
  • This is just a rewrite to make it more readable (probably doesn't look like it) and more maintainable (you can easily modify the values at the beginning of the code).
  • It won't run any faster since the 'chokepoint' is the multiple calls to the updateSD procedure.
  • To make it faster, you would want to write the values from the range to an array (e.g. Data = rg.Value) and modify the values in the array before writing them back to the range (e.g. rg.Value = Data). For the formatting (highlighting), you could combine the cells into a range using Union and then format the combined range in one go (e.g. urg.Interior.Color = vbYellow).
Sub Test()
    
    ' 1.) At the beginning of the procedure.
    
    ' Note that if you add more 'wsNames',
    ' you need to add more arrays to 'Vols'.
    Dim wsNames() As Variant
    wsNames = VBA.Array("TEST-3", "TEST-8")
    Dim RateValues() As Variant: RateValues = VBA.Array(50, 50, 100, 200)
    Dim Vols() As Variant: Vols = VBA.Array( _
        VBA.Array(95, 98, 110, 110), _
        VBA.Array(98, 98, 125, 125))
    Dim SweepValues() As Variant: SweepValues = VBA.Array( _
        VBA.Array(49.8, 99.8, 199.4), _
        VBA.Array(50.2, 100.2, 200.4))
    
    ' 2.) Code before the loop ???
    
    ' 2.a.) ???
        
    ' 2.b.)
    Dim Index As Variant: Index = Application.Match(wsname, wsNames, 0)
    Dim OutOfBounds As Boolean
        
    ' 3.) The loop
    For WhatEver ...
    
    ' 3.a.) Inside the loop:
    
        If IsNumeric(Index) Then
            Index = Index - 1 ' zero-based
            Select Case rate_value
            Case Is < RateValues(0)
                vol = Vols(Index)(0)
                MsgBox "Less than 50."
            Case RateValues(1)
                vol = Vols(Index)(1)
                sweep_value = SweepValues(0)(0)
                sweep_value_max = SweepValues(1)(0)
            Case RateValues(2)
                vol = Vols(Index)(2)
                sweep_value = SweepValues(0)(1)
                sweep_value_max = SweepValues(1)(1)
            Case RateValues(3)
                vol = Vols(Index)(3)
                sweep_value = SweepValues(0)(2)
                sweep_value_max = SweepValues(1)(2)
            Case Else ' rate_value not found?
                OutOfBounds = True
            End Select
        Else ' wsName not found?
            OutOfBounds = True
        End If
        
        If Not OutOfBounds Then
            
            updateSD sysnum, vol_rowindex, Max, vol
            updateSD sysnum, vol_rowindex_1, Max, vol
            
            ' ???
            updateSD sysnum, rate_rowindex, typ, rate_value
            updateSD sysnum, rate_rowindex_1, typ, rate_value
            updateSD sysnum, rate_rowindex_1, Min, sweep_value
            updateSD sysnum, rate_rowindex_1, Max, sweep_value_max
        
        End If

    Next WhatEver

End Sub
VBasic2008
  • 44,888
  • 5
  • 17
  • 28