VBA-tools / VBA-Dictionary

Drop-in replacement for Scripting.Dictionary on Mac
MIT License
348 stars 89 forks source link

Run-Time Error '438' - Object doesn't support this property or method #41

Closed kirinelf closed 9 months ago

kirinelf commented 9 months ago

Hi all, got a bit of a pickle! I've been tasked with making a script that can run on both Windows and MacOS, however part of the script depends on Microsoft Scripting Runtime. My search across the internet has lead me here, but I'm still getting issues.

What the script should do: Look at B61 and downwards to check for duplicates, when duplicates are found merge the rows.

The script I have at the moment runs perfectly fine with MSR, but doesn't work with the Dictionary imported from GitHub. It's located here: https://pastebin.com/36tAvLaa

Specifically, Debug points to Line 28: For Each itm In dc.

I am a completely novice scripter: This script was actually formed by me prompting ChatGPT until I got something usable, and as mentioned, it works perfectly fine in Windows with Microsoft Scripting Runtime. I hope I'll be able to get it running with the Dictionary here!

Thanks!

DecimalTurn commented 9 months ago

Can you provide the error message that pops up on MacOS?

Also, did you make sure that the Dictionary.cls file was imported as a class module and not a regular module? ie. Your project should look like this: image

Nick-vanGemeren commented 9 months ago

This issue is a duplicate of issue #17. The solution for you is to add the Keys method For Each itm In dc.Keys (in both places)

However, I see some potential problems in the script.

Theacdictionary stores the first row found for each B-column value (call it row A). Thedcdictionary stores the first duplicate row for each row A (call it row D). That's fine if you are sure that there will not be more than one duplicate. If not, then you need to turn the definition round and store row A for each duplicate row: If Not dc.Exists(i) Then dc.Add i, ac(tr) and adapt the following loops to match.

In the merge loop, you should use the length of the source row, unless you are sure that the target row will not be shorter. Otherwise trailing values will not be merged.

Do you need to merge columns to the left of B? If not, start from column C (i = col + 1)

=================== If this solves your problem, please close the issue here. If you need further help, let us know.

Nick-vanGemeren commented 9 months ago

Some further thoughts:

ThelastRowtest is not necessary. The loop(s) will fall through and there will be nothing to delete.

You don’t really need thedcdictionary at all. You can merge the duplicates during the scan. Deleting duplicates still has to wait until the end.

As written, the merge fills in blank fields in row A. But what about unequal values in rows A and D – which should be retained? If duplicates represent more recent info, then presumably row D non-blank values should overwrite those in row A. Check which merge method is appropriate.

The starting row is labelled “HDR”. This suggests that it is a header row and that header rows may occur further down (and be merged). If header rows (maybe from a new release of imported data) define data fields in a different column order, the result may be irreversibly scrambled data. You should build in strong defences to stop this happening.

If there is a large volume of data to merge, the runtime may be significant. You can keep the user informed by using either the StatusBar or a full-blown progress bar form. For-Each loops are more efficient than simple For loops, especially when working on a range of cells.

If blank lines in the merge area cause a cosmetic problem, they can be removed via thedRowsmechanism.

If your current script works, that’s great. If it needs extension, you might find (cautious) inspiration in the code below:

Option Explicit

Sub mergeRows()
    Const HDR As Long = 61 ' Header row
    Const col As Long = 2 ' Column used for merging rows
    Dim ws As Worksheet, lastRow As Long, i As Long
    Set ws = ThisWorkbook.Worksheets("ALS Import")
    lastRow = ws.Cells(ws.Rows.Count, col).End(xlUp).Row

    Dim ac As New Dictionary
    Dim dRows As Range: Set dRows = Nothing ' list of cells/rows to delete
    Dim tr As String
    Dim icell As Range

    Application.ScreenUpdating = False

 ' Find duplicate values in the chosen column
    For Each icell In Range(ws.Cells(HDR, col), ws.Cells(lastRow, col))
        tr = Trim(icell.Value)
        If Len(tr) > 0 Then
            If Not ac.Exists(tr) Then
                ac.Add tr, icell.Row
            Else
                ' If the key already exists, we have a duplicate to merge
                MergeDuplicate ws.Rows(icell.Row), ws.Rows(ac(tr)), 1
                ' list cells/rows to delete in a range (no deletes allowed during scan)
                MarkforDelete dRows, icell
            End If
        End If
    Next icell

    If Not dRows Is Nothing Then dRows.EntireRow.Delete
    Application.ScreenUpdating = True
End Sub

Sub MergeDuplicate(srcRow As Range, destRow As Range, startCol As Long)
' This merge assumes that the source row has younger/more accurate info to be retained.
    Dim mcell As Range
    Dim tr As String
    Dim startCell As Range
    Dim lastCell As Range
    With srcRow
        Set startCell = .Cells(1, startCol)
        Set lastCell = .Cells(1, .Columns.Count).End(xlToLeft)
    End With
    For Each mcell In Range(startCell, lastCell)    ' Scan source row
        tr = Trim(mcell.Value)
        If Len(tr) > 0 Then destRow.Cells(1, mcell.Column).Value = tr
    Next mcell
End Sub

Sub MarkforDelete(ByRef dRows As Range, icell As Range)
    If dRows Is Nothing Then Set dRows = icell      ' Can't union with nothing
    Set dRows = Union(dRows, icell)
End Sub
kirinelf commented 9 months ago

Thanks for all the help! I'll test things out when I get into work tomorrow. I'm an absolute scripting novice so most of what you said regarding the scripting went over my head, but I'll try and dive into the code and see if I can't piece together the differences between what ChatGPT gave me and what you have there, and figure out what they do!

I'll close this after I've tested things, hopefully they work!

@DecimalTurn: Unfortunately I do not have a Mac, I've basically been going over to another user and getting them to trial the script, and if it errors they just tell me it errors out. I'll have to get them to show an instance of the error and maybe take a screenshot. That said, there's quite a bit of progress in this thread, so hopefully that may not be needed! But I can definitely verify that the file was imported properly.

@Nick-vanGemeren

That's fine if you are sure that there will not be more than one duplicate.

Nice catch! In my experience working with the client data, there's never been more than one duplicate. However, we never know if this might change, so an additional check never hurts!

In the merge loop, you should use the length of the source row, unless you are sure that the target row will not be shorter.

Thus far, the rows have always been the exact same length. What it actually is is a single set of data that has somehow been split into two rows by the client, so my target is to re-merge the rows.

Do you need to merge columns to the left of B? If not, start from column C (i = col + 1)

No, everything to the left of B is the same, and contains populated data anyway. As mentioned above, the data is from the one source, it's just been split for some reason, and columns to the left of B are the names of the sources.

ThelastRowtest is not necessary. The loop(s) will fall through and there will be nothing to delete.

You don’t really need thedcdictionary at all. You can merge the duplicates during the scan. Deleting duplicates still has to wait until the end.

These two I'm unsure of, they were simply what was popped up to me by ChatGPT, so I ran with them.

But what about unequal values in rows A and D – which should be retained?

This is a very good point, as mentioned in my reply to the second bit I quoted from you, the data is just a single data split into two. For my particular use-case, there should never be a case where I'm merging two populated cells. One or the other will be empty. However, I do not know if the client will ever change the way the data is being presented, so in that case, I would prefer for the greater value to be taken.

The problem is that the data is in two forms: Either a string consisting of a "<" and a number, or a number. So while choosing between 6 and 8 is easy, I'd be interested in how to choose between <5 and 7. Or perhaps it's as simple as "If all cells show "<", take the first. If any cell does not contain "<", take the highest number"? Will have to faff around to figure out how that would work if it comes to it.

The starting row is labelled “HDR”. This suggests that it is a header row and that header rows may occur further down (and be merged)

Fortunately, this is one I don't have to wonder about, there will only ever be one header row!

If there is a large volume of data to merge, the runtime may be significant. You can keep the user informed by using either the StatusBar or a full-blown progress bar form. For-Each loops are more efficient than simple For loops, especially when working on a range of cells.

That's a good idea, I never really figured out how to implement things like that. I'll look into it!

Thanks again for all your inputs!

kirinelf commented 9 months ago

I ended up using @Nick-vanGemeren's solution, with one slight change: I was still getting a 438 error on line 25:

MergeDuplicate ws.Rows(icell.Row), ws.Rows(ac(tr)), 1

This was fixed by change "ac" to "ac.Item", and the script works as far as I can tell, at least on Windows. Now I'll try and find someone with a Mac to give it a spin, and it should be good to close! Thanks again for all your help!

kirinelf commented 9 months ago

Everything works on the platforms we have, and we have successfully done the conversions we need all the way into production! I was even able to reverse engineer the row merging to have another module for column merging. Thank you so much for all your help! Consider the issue closed~