1
\$\begingroup\$

I have always wanted to start unit testing my code, and ending last week I downloaded Rubberduck again (first time there was just too little information how to do it to get me going) but after watching this video unit testing, I decided that it looks easy enough.

So the whole weekend I read up more and watched more videos and got more excited - because Monday I am going to go through my latest project and start incorporating unit tests!!

Or maybe not... I got to this first class of mine, and I really do not see a lot of places that I can do unit testing - meaning not a lot of return functions with a value to test.

The long and short of this class is that I inherited a worksheet with a lot of data all over the place, so much so that I can not even add or delete a line without breaking a lot of formulas (true story). Anyway, I made a new sheet, loaded information from a DB into that sheet, and from there on I am doing a data transfer to the infected patient (as mentioned above) every time I need to update the information.

My class with methods is below and I am wondering if there is anyway to run a test to check if data are transferred correctly, or to re-write the code to make it less brittle. Please feel free to tear the code apart and give any other advice you deem necessary.

Option Explicit
Private firstTruckColm As Long
Private truckRow As Long
Private numberOfShops As Long
Private numberOfProducts As Long
Private wb As Workbook
Private laailys As Worksheet
Private bloem As Worksheet
Private bloemShopNamesStartRow As Long
Private bloemShopNamesStartColm As Long

Public Sub loadTrucksInit()

    Call initValues
    Call insertShopNamesAndProd

End Sub

Private Sub initValues() 'Loads values from a "settings" module. (Any thoughts on this)'

    firstTruckColm = mdl_settings.returnLaailysFirstTruckColm
    truckRow = mdl_settings.returnLaailysTruckRow
    numberOfShops = mdl_settings.returnNumberOfBloemShops
    numberOfProducts = mdl_settings.returnNumberOfBloemProducts
    Set wb = ThisWorkbook
    Set laailys = wb.Sheets("Laailys")
    Set bloem = wb.Sheets("Bloem")
    bloemShopNamesStartRow = mdl_settings.returnBloemShopNamesStartRow
    bloemShopNamesStartColm = mdl_settings.returnBloemShopNamesStartColm

End Sub

Private Sub insertShopNamesAndProd()
    Dim a As Long
    For a = 1 To numberOfShops
        With laailys
            .Cells(truckRow, firstTruckColm + a - 1).Value = bloem.Cells(bloemShopNamesStartRow, bloemShopNamesStartColm + a).Value
        End With
        Call goThroughProducts(bloemShopNamesStartColm + a, firstTruckColm + a - 1)
    Next a
End Sub

Private Sub goThroughProducts(colmToUseBloem As Long, colmToUseLaailys As Long)
    Dim a As Long
    For a = bloemShopNamesStartRow + 1 To numberOfProducts + bloemShopNamesStartRow
        If bloem.Cells(a, colmToUseBloem).Value <> vbNullString Then
            Call insertProducts(bloem.Cells(a, 1).Value, bloem.Cells(a, colmToUseBloem).Value, colmToUseLaailys)
        End If
    Next a
End Sub

Private Sub insertProducts(productCode, prodAmount, colmToUseLaailys)
    Dim a As Long
    Dim checker As Boolean
    checker = False
    For a = 1 To 200
        If laailys.Cells(a, 4).Value = productCode Then
            laailys.Cells(a, colmToUseLaailys).Value = prodAmount
            checker = True
            Exit Sub
        End If
    Next a
    If checker = False Then
        MsgBox productCode & " did not read in correctly, make sure the product code in 'Laailys' is the same as in 'Bloem'"
    End If
End Sub

A few other questions that I also want to know if possible:

  1. I like to have a initValues method that loads all the properties that I am going to use in my class all at once in one neat place if I need to change any. Is this an acceptable practice?

  2. I point to colm values like firstTruckColm + a - 1, should I first declare another variable and give this value to that variable, just so that my arguments for other procedures look better?

  3. Should I rather change my Subs to Functions with a return value as most I can?

\$\endgroup\$
  • \$\begingroup\$ Also, a few weeks ago I read a post by RubberDuck (I think) where he explained how to refracture an example that he gave of where you get data from an Access DB and use it in a sheet (or vice versa) in such a way that you can unit test it, where you couldn't do it with the original code. I can not find it for the life of me now, if someone knows what I am talking about please link it for me. I immediately like the idea when I read it. \$\endgroup\$ – Alfa Bravo Apr 9 '18 at 13:04
  • \$\begingroup\$ Maybe you're thinking of rubberduck vba, a project that includes unit testing and has documentation on how to use it? One of the developers is rubberduck. mat also has a lot of answers in that realm \$\endgroup\$ – Raystafarian Apr 10 '18 at 22:11
  • \$\begingroup\$ Oh, Thomas posts some pretty good ideas like what you mention as well. \$\endgroup\$ – Raystafarian Apr 11 '18 at 1:58
  • \$\begingroup\$ Hi Raystafarian, I was def thinking of RDVBA and I am using it at the moment, but I looked every where on the site and related documentation to see if I could find it again. In the end I realized I must have looked at the page at home and not at work, so when I went through search history there I got it: [How to unit test][1]. [1]: rubberduckvba.wordpress.com/2017/10/19/… \$\endgroup\$ – Alfa Bravo Apr 11 '18 at 5:27
  • \$\begingroup\$ You can also visit the wiki - github.com/rubberduck-vba/Rubberduck/wiki/Unit-Testing \$\endgroup\$ – Raystafarian Apr 11 '18 at 5:40
0
\$\begingroup\$

You say this is a class, but you don't have anything exposed from it. What I mean is if I create an instance of this class, there's nothing I can do with it. I can't Get or Let any properties and I can't use any of the Private methods.

Call

You don't need to Call Sub anymore. You also don't need to enclose your arguments. E.g.

goThroughProducts somecolumn + a, somecolumn + a - 1

Worksheets

Worksheets have a CodeName property - View Properties window (F4) and the (Name) field (the one at the top) can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

Naming

Maybe some of this isn't in English and so I don't understand the meanings, but your naming could be improved. I have no idea what a laailys worksheet could be. Also you use product and prod both. Try to stay consistent and clear with your naming. What is checker checking? It's boolean so normally you would name it something like isMatch.

Also a Boolean is initialized as False so no need to set it as false.

Class

It seems strange to have a class refer to ThisWorkbook. What is your goal with this class? I don't see anything that can't be done with already existing objects.

ByVal

All your arguments are being passed ByRef implicitly. You want to pass ByVal whenever possible. e.g.

Private Sub InsertProducts(ByVal productCode As Long, ByVal productAmount As Long, ByVal targetColumn As Long

Typing

You'll notice I gave your variables a type above. Otherwise they are implicitly variants, which are not needed.

Loop

In InsertShopNamesAndProd you have a call to a procedure within a loop and a with:

For a = 1 To numberOfShops
        With laailys
            .Cells(truckRow, firstTruckColm + a - 1).Value = bloem.Cells(bloemShopNamesStartRow, bloemShopNamesStartColm + a).Value
        End With
        Call goThroughProducts(bloemShopNamesStartColm + a, firstTruckColm + a - 1)
    Next a

And in that procedure you have another call within a loop -

For a = bloemShopNamesStartRow + 1 To numberOfProducts + bloemShopNamesStartRow
        If bloem.Cells(a, colmToUseBloem).Value <> vbNullString Then
            Call insertProducts(bloem.Cells(a, 1).Value, bloem.Cells(a, colmToUseBloem).Value, colmToUseLaailys)
        End If
    Next a

That seems pretty repetitive. Instead think about refactoring these procedures so that you take all the products into an array, go through your shops and insert products as you work.

If

Here

If bloem.Cells(a, colmToUseBloem).Value <> vbNullString Then
            Call insertProducts(bloem.Cells(a, 1).Value, bloem.Cells(a, colmToUseBloem).Value, colmToUseLaailys)
        End If

Can be put on one line. And here

Dim checker As Boolean
    checker = False
    For a = 1 To 200
        If laailys.Cells(a, 4).Value = productCode Then
            laailys.Cells(a, colmToUseLaailys).Value = prodAmount
            checker = True
            Exit Sub
        End If
    Next a
    If checker = False Then
        MsgBox productCode & " did not read in correctly, make sure the product code in 'Laailys' is the same as in 'Bloem'"
    End If

I'm not sure how you can get to that second If without checker being false.

You can also use a boolean in an if as the actual test:

If Not checker Then

Settings module

You use your "settings" module to get objects and rows? That doesn't sound right. Settings should be loaded and stored so they can be reset. What you're doing is populating so naming it settings isn't entirely accurate.

\$\endgroup\$
  • \$\begingroup\$ Hi Ray, first of all thank you for the time you have put into answering my question - I am systemically going through every point you have made and will leave a comments as I go. Firstly, on you first question, I run the class in my form (or the press of a button) by calling the public loadTrucksInit method, which then runs everything in the class and does what I want it to do. I think my big problem is still that I may be using classes just like modules - is that what it looks like to you too? \$\endgroup\$ – Alfa Bravo Apr 12 '18 at 7:35
  • \$\begingroup\$ Ah, on a form. Yeah, Using a Class is like creating an Object (like Range, Variant, Dictionary). So if there's already an object that does everything you need, there's no real need for a class. You can still make the class, but usually it's to combine several different properties into one object. \$\endgroup\$ – Raystafarian Apr 12 '18 at 22:03

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.