Simplifying VBA Code

Question

I wrote a fully working VBA macro that goes through inputed client ID's and returns the specified commission fee for that client.

The code 100% works, however I need to have it so that my boss is able to easily add new clients with their special commission %'s to the list, without having to go through the VBA code.

How can I make a simplified VBA macro or just type an Excel function to do exactly what my code below does, but make it versitile to the special klient table?

The main Excel table looks like this:

The table with the special klients, commission fees, currency, markets, looks like this:

Option Explicit

Sub komisijas_calc_Click()

'Declare the variables
Dim klienta_nr As Long
Dim ISIN As String
Dim Cena As Double
Dim Skaits As Double
Dim Komisija As Double
Dim vk As String
Dim Summa As Double
Dim kSheet As Worksheet

Dim lngFirstRow As Long: lngFirstRow = 2
Dim lngLastRow  As Long
Dim lngCol      As Long: lngCol = 2
Dim lngCounter  As Long
Dim lngKom As Long: lngKom = 11




Set kSheet = ThisWorkbook.Sheets("spec_klienti")


With Worksheets(1)
    lngLastRow = .Cells(.Rows.Count, lngCol).End(xlUp).Row

    For lngCounter = lngFirstRow To lngLastRow

    klienta_nr = Range("B" & lngCounter).Value
    ISIN = Range("E" & lngCounter).Value
    Cena = Range("H" & lngCounter).Value
    Skaits = Range("I" & lngCounter).Value
    vk = Range("D" & lngCounter).Value
    Summa = Cena * Skaits


Select Case klienta_nr



    Case 111111
                '(Vācija, Francija, Nīderlandes, Itālija, Īrija) - 30 EUR MIN
                If klienta_nr = 111111 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                If klienta_nr = 111111 And Komisija <= 30 Then
                    .Cells(lngCounter, lngKom) = 30
                        End If


                If klienta_nr = 1111111 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
                    Komisija = Summa * 0.003
                    If Komisija >= 40 Then
                        .Cells(lngCounter, lngKom) = 40
                            End If
                        End If


    Case 2222222
                '(Vācija, Francija, Nīderlandes, Itālija, Īrija) - 30 EUR MIN
                If klienta_nr = 222222 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                'Set 30 EUR Min
                If klienta_nr = 2222222 And Komisija <= 30 Then
                    .Cells(lngCounter, lngKom) = 30
                        End If

                If klienta_nr = 2 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
                    GoTo AllElseFails
                        End If


Case Else

AllElseFails:

              'IP2, 0.03% komisija, 40 EUR/USD Max
                If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then
                    Komisija = Summa * 0.003
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
              'IP1, 0.1% komisija, 40 EUR/USD Max
                If Right(vk, 1) = 7 Then
                    Komisija = Summa * 0.01
                    .Cells(lngCounter, lngKom) = Komisija
                        End If
                'Komisija MAX is 40, so anything >=40 equals 40
                If Komisija >= 40 Then
                    .Cells(lngCounter, lngKom) = 40
                        End If
            'End If
     End Select
   Next lngCounter
  End With
End Sub

Answer 1

Several observations about the code not necessarily related to the (not so) explicit question:

Formatting

Have some consistent indentation of your code, it will make it much easier to read. Rubberduck is a great tool for that, and it will help you find all sorts of spots for other VBA code improvements. (Happy user, haven't yet contributed to the project.)

Remove excessive vertical spacing as it allows you to fit more of this lengthy procedure on the screen.

Variable Naming

Your first set of variables seem reasonably well named (though I don't speak... whatever language you've named them in :) until... we get to vk - This seems to be the commission percentage so why not call it CommissionPercent?

Your second set of variables, however, leave a bit to be desired in the choice of names.

Drop the Hungarian Notation unless you're going to do it usefully. (Most people don't, so you're not alone!)

FirstRow is just as intuitive a name as lngFirstRow and when you're using it, knowing that it's a Long doesn't add value. This applies to all the lng prefixed variables.

lngCol - when I look at that, I'm going to assume that it's something like "the current column I'm processing", but looking through the code, it's actually a pointer to the Client column, and it never changes, so how about Const CLIENT_COL as Long = 2 instead.

The same can be done with lngKom - make it a Const called I'M_NOT_SURE_COL (Col 11 is col K, but that's a somewhat random column in your "Market" section. Currently only set to "EE", but that could certainly change, I'd think).

Clean up the Select statements

In this code:

Select Case klienta_nr
  Case 111111
  '(Vacija, Francija, Niderlandes, Italija, Irija) - 30 EUR MIN
  If klienta_nr = 111111 And (Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE") Then

The If klienta_nr = 111111 And is totally unnecessary. Because you're inside the Select Case and this is Case 111111, klienta_nr will, by definition, be 111111. Remove that part to simplify the code a bit.

Actually, eliminate all the "Magic Numbers" then clean up the Select statements

In your Select you have Case 111111 (6 ones). Inside that Case, you have If 111111 (6 ones), followed by If 111111 (6 ones), followed by If 1111111 (7 ones). This appears, to the innocent bystander, to be a typo on the last If statement. However, since all the rest of the conditions are the same, maybe it's intentional. In either case, it's confusing for the next programmer to come along (where "Next Programmer" is often "Future You"). If you declare some Const ABC_CLIENT_ID as Long = 111111 and Const DEF_CLIENT_ID as Long = 1111111, at least "next programmer" will know that this isn't a typo since you've intentionally declared 2 separate constants for the different values.

Be aware that your code will never fall into the 3rd If statement. By definition, when the code falls into the Case 111111 (6 ones) section of the Select, If klienta_nr = 1111111 (7 ones) will never be True. This is dead code (as written) and should be removed, or the typo fixed to make it do what you intend.

The same can be said for the Case 2222222 (7 twos) section. The only If that will ever execute is the middle one under the 'Set 30 EUR Min comment. The first If only has 6 twos, which will never evaluate to True and the last has only 1 two.

Eliminate GoTo

The good news about that last If statement never executing is that you'll never hit the GoTo AllElseFails jump! If you really need to execute the AllElseFails code at some point in the Case 2222222 section, then refactor your code like this:

Private Sub AllElseFails(ByVal vk As String, ByRef Komisija As Double, ByVal lngCounter As Long, ByVal lngKom As Long, ByVal Summa As Double, ByVal WkSht As Worksheet)

  'IP2, 0.03% komisija, 40 EUR/USD Max
  If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then
    Komisija = Summa * 0.003
    WkSht.Cells(lngCounter, lngKom) = Komisija
  End If
  'IP1, 0.1% komisija, 40 EUR/USD Max
  If Right(vk, 1) = 7 Then
    Komisija = Summa * 0.01
    WkSht.Cells(lngCounter, lngKom) = Komisija
  End If
  'Komisija MAX is 40, so anything >=40 equals 40
  If Komisija >= 40 Then
    WkSht.Cells(lngCounter, lngKom) = 40
  End If
End Sub

And call AllElseFails when you need that code instead of jumping around in a Select Case structure like this:

      If klienta_nr = 2 And (Left(ISIN, 2) <> "DE" Or Left(ISIN, 2) <> "FR" Or Left(ISIN, 2) <> "NL" Or Left(ISIN, 2) <> "IT" Or Left(ISIN, 2) <> "IE") Then
        AllElseFails vk, Komisija, lngCounter, lngKom, Summa, Worksheet(1)
      End If
    Case Else
      AllElseFails vk, Komisija, lngCounter, lngKom, Summa, Worksheet(1)

That's the way to reuse the code without getting the "next programmer" very confused.

As an added tweak to the refactored Function, you have:

  If Right(vk, 1) = 1 Or Right(vk, 1) = 8 Then

followed by

  If Right(vk, 1) = 7 Then

You should probably change that second If statement to an ElseIf. There's no need to execute the check for 7 if you've already determined it's a 1 or 8.

Refactor to eliminate redundant code:

You have

(Left(ISIN, 2) = "DE" Or Left(ISIN, 2) = "FR" Or Left(ISIN, 2) = "NL" Or Left(ISIN, 2) = "IT" Or Left(ISIN, 2) = "IE")

all over the place. This is a great opportunity to create a Function that does all that testing for you and returns a Boolean:

Private Function IsADesiredISIN(ByVal ISIN As String) As Boolean

    Select Case ISIN
    Case "DE", "FR", "NL", "IT", "IE"
      IsADesiredISIN = True
    Case Else
      IsADesiredISIN = False
    End Select

End Function

Then your if statements become:

If klienta_nr = 111111 And IsADesiredISIN(Left(ISIN, 2)) Then
If klienta_nr = 222222 And IsADesiredISIN(Left(ISIN, 2)) Then

Which should really be

If IsADesiredISIN(Left(ISIN, 2)) Then

since the klienta_nr = 111111 and klienta_nr = 222222 are redundant (as mentioned above).

And, as an added bonus, if you need to include a new country code in the list of ISIN you're processing, you do it in one place and it effects all locations meaning you don't accidentally forget one.

---

This really hasn't addressed your actual question (I don't think), but it does simplify the code by eliminating some questions that the "next programmer" may have about what the code does and why.