3

I was trying to use a with statement since they are faster than a loop.

There are 72,000 rows, the exact number can vary. An item code needs to go in column A depending on the currency code in column B.

I am referencing a collection to retrieve the code based on the currency code. What is the fastest way I can accomplish this? Here is my code... which doesnt work.

Sub Collector()

Dim cn As Collection
Dim LastRow As Long
Dim cur As Long
Dim destws As Worksheet

Set destws = ThisWorkbook.Worksheets("Data")

Set cn = New Collection
cn.Add "120000037650264", "AUD"
cn.Add "140000028802654", "CAD"
cn.Add "106000061411232", "CHF"
cn.Add "100700037144679", "CNY"
cn.Add "108000077165454", "EUR"
cn.Add "100900028865402", "GBP"
cn.Add "100700034152263", "HKD"
cn.Add "103000037165403", "HUF"
cn.Add "100400055172256", "INR"
cn.Add "100090035614270", "JPY"
cn.Add "100600035472288", "KRW"
cn.Add "100040036172267", "MXN"
cn.Add "100004036162300", "PLN"
cn.Add "121000037176585", "RUB"
cn.Add "133000040272294", "THB"
cn.Add "100430020172276", "TWD"
cn.Add "109790029172291", "UAH"
cn.Add "100004007305201", "USD"
cn.Add "100003051687277", "ZAR"

LastRow = destws.Cells(Rows.Count, 2).End(xlUp).Row


    With destws.Range("A2:A" & LastRow)
        .Value = cn.Item(Cells(cur, 2).Value) 'generates object defined error
    End With

End Sub

Example: I want cell A2 to have a value of 100004007305201 if cell B2 value is USD.

Any help would be greatly appreciated!

Tom B.
  • 31
  • 5
  • 1
    Where do you define `cur` ? – DecimalTurn Feb 02 '18 at 16:47
  • Tnank you @DecimalTurn That's probably my problem, at least part of it. How do I define `cur` in a way that it is relative? – Tom B. Feb 02 '18 at 16:50
  • Why don't you just use VLOOKUP? – SJR Feb 02 '18 at 16:52
  • @SJR It's part of a much larger macro that is doing a lot more than this task. Is it efficient to use `FormulaR1C1` and put vlookups in column A keeping in mind there could be up to 72,000 of them? – Tom B. Feb 02 '18 at 16:56
  • 3
    Probably quickest to store the results in an array and then write to the spreadsheet in one go. – SJR Feb 02 '18 at 16:57
  • Thank you @SJR! I am a slow learner, and I still do not know how I would store the results in an array and write to the spreadsheet in one go. – Tom B. Feb 02 '18 at 17:09
  • @TomB. I added the array method in my answer, if you want to have a look. – DecimalTurn Feb 02 '18 at 17:24

4 Answers4

5

Accessing Collection items by index is definitely a performance issue. Collections want to be iterated in a For Each loop! If you know in advance how many items you'll need, best use an array; accessing array items by index is exactly what arrays do best (and that's why they're best iterated with a For loop).

Writing to a Range in a loop is also highly inefficient.

Now, you're not dumping collection/array items into a Range - you're looking up key/value pairs. The single most efficient way to do this is with a Dictionary. A Collection can be keyed (as you did) too, but I like calling a cat a cat, so I use a Dictionary for key-value pairs.

Note: I'm going to assume your key/value pairs are account/currency. Adjust as needed; the idea is to name things, so that the code speaks for itself.

You could have a Private Function CreateAccountsByCurrencyDictionary that creates, populates and returns a Dictionary, and then your macro could have a Static local variable (so that it's not uselessly re-initialized every time the macro is invoked) to hold it:

Static accountsByCurrency As Scripting.Dictionary 'reference Microsoft Scripting Runtime
If accountsByCurrency Is Nothing Then
    Set accountsByCurrency = CreateAccountsByCurrencyDictionary
End If

Then you grab your working range and dump it into a 2D array - the simplest way is to have your data live in a ListObject (i.e. a named table); you can easily convert your range into a table by selecting "format as table" from the Home Ribbon tab - then you don't need to track where the last row is, the table does it for you!

Here Sheet1 is the code name of the worksheet you need to work with. Always qualify Range calls with a specific worksheet object. By using the sheets' code name, you make your code work regardless of what the ActiveSheet is.

Dim target As Range
Set target = Sheet1.ListObjects("TableName").DataBodyRange

Dim values As Variant
values = target.Value

Now that you have a 2D array (values), iterate it with a For loop and do your lookups:

Dim currentRow As Long
For currentRow = LBound(values, 1) To UBound(values, 1)

    ' never assume you're looking at valid data
    Dim currentKeyValue As Variant
    currentKeyValue = values(currentRow, 1)
    Debug.Assert Not IsError(currentKeyValue) ' there's a problem in the data

    ' key is a valid string, but might not exist in the lookup dictionary
    Dim currentKey As String
    currentKey = currentKeyValue
    If accountsByCurrency.Exists(currentKey) Then
        ' lookup succeeded, update the array:
        values(currentRow, 1) = accountsByCurrency(currentKey)
    Else
        Debug.Print "Key not found: " & currentKey, "Index: " & currentRow
        Debug.Assert False ' dictionary is missing a key. what now?
    End If
Next

If all goes well the values array now contains your corrected values, you can update the actual worksheet - and since you have the values in a 2D array, that's a single instruction!

target.Value = values

The CreateAccountsByCurrencyDictionary function might look something like this:

Private Function CreateAccountsByCurrencyDictionary() As Scripting.Dictionary
    Dim result As Scripting.Dictionary
    Set result = New Scripting.Dictionary
    With result
        .Add "AUD", "120000037650264"
        .Add "CAD", "140000028802654"
        '...
    End With
    Set CreateAccountsByCurrencyDictionary = result
End Function

Or, the values could be populated from another worksheet table instead of being hard-coded. Point being, how the lookup values are acquired is a concern in its own right, and belongs in its own scope/procedure/function.

Mathieu Guindon
  • 69,817
  • 8
  • 107
  • 235
  • Wow - thank you so much for the time you took to write the useful response. I am going to sit down with this until i understand all of it because I think there are many key points in it that I need to learn to advance my vba. The only issue is that I cannot write the table into another worksheet due to the nature of the file I'll be sending to people. – Tom B. Feb 02 '18 at 17:52
  • @TomB. note that none of it is tested; code provided for illustrative purposes only, please don't just copy-paste =) ...that said I'm not sure what you mean by "cannot write the table into another worksheet", ..it's the same sheet. – Mathieu Guindon Feb 02 '18 at 17:53
  • I should have taken the time to understand your post before commenting that! I look forward to sitting down with it and some tea tonight after work! – Tom B. Feb 02 '18 at 18:06
1

My initial thougth was that you didn't define cur which could be defined as follow if you where only looking at one cell (A2):

With destws.Range("A2")
    cur = .Column + 1
    .Value = cn.Item(Cells(cur, 2).Value)
End With

But since you are looking at a lot of cells, it would be better to use an array to write to the cells all at once, which can highly increase the speed.

Sub Collector()

Dim cn As Collection
Dim LastRow As Long
Dim destws As Worksheet

Set destws = ThisWorkbook.Worksheets("Data")


Set cn = New Collection
cn.Add "120000037650264", "AUD"
cn.Add "140000028802654", "CAD"
cn.Add "106000061411232", "CHF"
cn.Add "100700037144679", "CNY"
cn.Add "108000077165454", "EUR"
cn.Add "100900028865402", "GBP"
cn.Add "100700034152263", "HKD"
cn.Add "103000037165403", "HUF"
cn.Add "100400055172256", "INR"
cn.Add "100090035614270", "JPY"
cn.Add "100600035472288", "KRW"
cn.Add "100040036172267", "MXN"
cn.Add "100004036162300", "PLN"
cn.Add "121000037176585", "RUB"
cn.Add "133000040272294", "THB"
cn.Add "100430020172276", "TWD"
cn.Add "109790029172291", "UAH"
cn.Add "100004007305201", "USD"
cn.Add "100003051687277", "ZAR"

LastRow = destws.Cells(Rows.Count, 2).End(xlUp).Row

Dim MyArray() As Variant
MyArray() = destws.Range("B2:B" & LastRow)

Dim i As Long
For i = 1 To UBound(MyArray,1)
    MyArray(i, 1) = cn.Item(MyArray(i, 1))
Next i

destws.Range("A2:A" & LastRow).Value2 = MyArray


End Sub
DecimalTurn
  • 3,243
  • 3
  • 16
  • 36
  • Thank you! When I did this, all the cells in column A returned the code related to the first entry in column B. Ex B2 was GBP, all of col A was 100900028865402 – Tom B. Feb 02 '18 at 17:02
  • @Xabier Yeah, I was focused on the example of the question with only one cell. But if it has to be for a lot of cells, I would use arrays. I'm going to edit my answer to reflect that. – DecimalTurn Feb 02 '18 at 17:07
  • 1
    This is a good solution that uses a `Collection` - my main concern with it would be the unqualified `Range` and `Cells` calls, which implicitly refer to whatever the `ActiveSheet` is (assuming the code is in a standard module), or implicitly refer to whatever worksheet's code-behind this is written in. I like code that *reads* the same to *run* the same, so I'd either fully qualify the `Range` and `Cells` calls with a specific worksheet's *code name*, or qualified with `Me` if it's in a worksheet's code-behind, to explicitly refer to that sheet. In any case, +1 for beating me to it! – Mathieu Guindon Feb 02 '18 at 17:44
  • @Mat'sMug I totally agree. I guess I was following the "spirit" of OP's code which didn't declare any workbook/worksheet object. I feel like it's sometime better to discuss one element at the time for new comers to avoid confusion, but that's surely something I could mention. – DecimalTurn Feb 02 '18 at 18:21
  • 1
    @Mat'sMug Thank you and DecimalTurn for your help. I edited the original post to specify the wb and ws. I need to get used to writing explicitly to avoid confusion going forward. – Tom B. Feb 02 '18 at 19:31
  • @TomB. no problem! Feel free to mark any answer as accepted (tick the hollow checkmark next to the voting buttons). Also see [this answer](https://stackoverflow.com/a/48589646/1188513) about retrieving worksheets. I doubt your `Data` sheet will live in `ActiveWorkbook` under all circumstances - that's likely a bug waiting to happen. `ThisWorkbook` is the book running the code, `ActiveWorkbook` is whatever workbook is currently active. – Mathieu Guindon Feb 02 '18 at 19:33
  • 2
    @Mat'sMug , awesome, I've learned so much on this one post. (didnt know that about ActiveWorkbook and ThisWorkbook) I was able to get DecimalTurn's code to work but I want to sit down with your solution later to see which is faster before I accept the answer! – Tom B. Feb 02 '18 at 19:45
0

How about this;

Sub Collector()

Dim cn As Collection
Dim LastRow As Long
Dim cur As Long
Dim ws As Worksheet: Set ws = Sheets("Sheet1")
'declare and set your worksheet, amend as required.
Set cn = New Collection
cn.Add "120000037650264", "AUD"
cn.Add "140000028802654", "CAD"
cn.Add "106000061411232", "CHF"
cn.Add "100700037144679", "CNY"
cn.Add "108000077165454", "EUR"
cn.Add "100900028865402", "GBP"
cn.Add "100700034152263", "HKD"
cn.Add "103000037165403", "HUF"
cn.Add "100400055172256", "INR"
cn.Add "100090035614270", "JPY"
cn.Add "100600035472288", "KRW"
cn.Add "100040036172267", "MXN"
cn.Add "100004036162300", "PLN"
cn.Add "121000037176585", "RUB"
cn.Add "133000040272294", "THB"
cn.Add "100430020172276", "TWD"
cn.Add "109790029172291", "UAH"
cn.Add "100004007305201", "USD"
cn.Add "100003051687277", "ZAR"

LastRow = ws.Cells(ws.Rows.Count, 2).End(xlUp).Row

Application.Calculation = xlManual
    For i = 2 To LastRow
        ws.Cells(i, 1).Value = cn.Item(ws.Cells(i, 2).Value)
    Next i
Application.Calculation = xlCalculationAutomatic
End Sub
Xabier
  • 7,587
  • 1
  • 8
  • 20
  • Thank you @Xabier - Is it your experience that `For` loops are as efficient as using `With` statements? – Tom B. Feb 02 '18 at 17:04
  • You can't simply write a different thing to each row with a With statement, but if you were to use a Vlookup with With, then you could lookup dynamically, the reason for the For Loop is as an alternative to using Arrays which would be faster, but would you notice the difference,... Not sure... – Xabier Feb 02 '18 at 17:06
  • 1
    I think that with up to 72,000 cells, it can make a notable difference to use an array. – DecimalTurn Feb 02 '18 at 17:21
  • @DecimalTurn I agree that Arrays will be faster, but how much faster I do not know... Would you be able to record a notable difference? Maybe, but I agree with using Arrays, just thought that my example would suffice for the OP's question... :) – Xabier Feb 02 '18 at 17:30
  • 2
    Well, the question asks about "the most efficient way", so I think a fully complete answer should address speed issue. – DecimalTurn Feb 02 '18 at 17:38
0

From a quick look, you use cur similar to that in a loop, which would go over your array and make the change, e.g.:

Dim cur as Long, lr as Long
lr = cells(rows.count, 1).end(xlup).row 'dynamic last row
For cur = 2 to lr step 1
    Select Case Cells(cur,3).Value
    Case "AUD"
        Cells(cur,2).value = "120000037650264"
    Case "" 'add in others
        Cells...blah blah blah        
    End Select
Next i

It would make the most sense, given you have a table with these values, to just use a formula with either a vlookup or index/match, e.g.:

'Where your table is on Sheet2 with Column A being the currency code (3-letter code) code and Column B being the item code
'Where you are working on Sheet1
=INDEX(Sheet2!B:B,MATCH(Sheet1!C1,Sheet2!A:A,0)) 'in column B for the active row
Cyril
  • 6,448
  • 1
  • 18
  • 31
  • thank you for your help! It woudl be best not to use vlookup or index match functions for this particular project. It would be easiest for the end user if the lookup was packaged into the code. I'll test the `for` loop to see if I can use this without performance issues. – Tom B. Feb 02 '18 at 17:27
  • @TomB. Copy. I would recommend the Select Case path, then, as it should be fairly quick; the real benefit is directly having that table of currency/item values within the code. Good luck! – Cyril Feb 02 '18 at 18:12