Cfrtjryk

Background: End users wanted to automate the process of taking information out of our CRM software (in the form of a csv), and then process it into something more usable for their purposes. Ended up not getting used, but I did manage to make it run/work on old data.

The following code expects an import of values to be placed in a sheet labeled "Import", each row of the import is associated with a specific location that has been inspected, but there are multiple rows per location (different comments, and information for the location).

The code is meant to take these multiple rows, and create an output with only one row per location, while concatenating the information that isn't duplicated in the import rows.

Wanted to see how many pitfalls I've fallen into, I've taken college coursework with programming, but no real guidance in industry (learned a great deal through trial/error when asked to make something at work).

Code does run, but I'm particularly interested in knowing whether my use of an array is really bad practice or not. I use a similar structure for a lot of the VBA code I make that needs to deal with import data (I don't know of a better way to manipulate the data, leaving it in the sheets ends up being excessively slow on larger sets of data).

Sub ProcessData()



    Dim Imp_Sheet As Worksheet

    Set Imp_Sheet = Sheets("Import")

    Dim Unique_Sheet As Worksheet

    Set Unique_Sheet = Sheets("UniqueVals")

    Dim Out_Sheet As Worksheet

    Set Out_Sheet = Sheets("Output")



    'Clear unique value sheet

    Unique_Sheet.Cells.Clear



    'Copy up to 3000 locations from the import sheet.

    Imp_Sheet.Range("C1:C3000").Copy

    Unique_Sheet.Paste (Unique_Sheet.Cells(1, 1))

    'Remove duplicate locations

    Unique_Sheet.Range("A1:A3000").RemoveDuplicates Columns:=(1), Header:=xlYes



    'Find number of inspections(Could include duplicate inspections that I don't want)

    Dim Inspection_Total As Long: Inspection_Total = Find_End(Unique_Sheet.Name)



    For i = 2 To Inspection_Total

        'Check if it's found the end of the unique value list

        If (Unique_Sheet.Cells(i, 1).Value = "") Then

            Exit For

        End If

        'give each unique value a number for future reference

        Unique_Sheet.Cells(i, 2).Value = i - 1

    Next i



    Unique_Sheet.Cells(1, 2).Value = i - 2

    'Change inspection total to be the actual number of inspections

    Inspection_Total = i - 2



    Dim Import_Total: Import_Total = Find_End(Imp_Sheet.Name)



    'Prepare array to load import values into.

    Dim Imp_Arr() As Variant

    '0 = location, 1= city, 2= st, 3=date site set, 4=pre-ins date, 5= mod name, 6= mod title, 7 = summary

    ReDim Imp_Arr(Inspection_Total, 7)



    'Walk through import sheet, for reach row, compare to the list of unique locations (to find where each row belongs)

    For i = 2 To Import_Total

        For x = 2 To Unique_Sheet.Cells(1, 2) + 1

            If (Imp_Sheet.Cells(i, 3) = Unique_Sheet.Cells(x, 1)) Then

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 0) = Imp_Sheet.Cells(i, 3)

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 1) = Imp_Sheet.Cells(i, 7)

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 2) = Imp_Sheet.Cells(i, 8)

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 3) = "MANUAL INPUT REQUIRED"

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 4) = Imp_Sheet.Cells(i, 6)

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 5) = Imp_Sheet.Cells(i, 4)

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 6) = Imp_Sheet.Cells(i, 5)

                    'Comments on a specific location need to be on one row, this accomplishes that

                    Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 7) = Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 7) & Imp_Sheet.Cells(i, 10) & "  " & Imp_Sheet.Cells(i, 11) & "  " & Imp_Sheet.Cells(i, 12) & ";" & Chr(10)

            End If

        Next x

    Next i



    'Output one row for each location.

    For i = 0 To Inspection_Total



        For Z = 0 To 7

            '

            Out_Sheet.Cells(i + 2, Z + 1).Value = Imp_Arr(i, Z)



        Next Z



    Next i



End Sub

Variables

You did a good job of naming your variables as what they are, but you didn't declare some.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type. Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

Also, you can do better with the 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("Import") and instead just use Import.

You also ran into the pitfall of VBA naming style. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

You are missing a function Find_End which I assume just find the last row. So it would be something like this

Dim inspectionLastRow As Long

inspectionLastRow = UniqueSheet.Cells(Rows.Count, 1).End(xlUp).Row

Now you have your loops. You have an i loop, a x loop and a Z loop. In this case I would give them more specific names -

For inspectionIndex = 2 To inspectionLastRow

For importIndex = 2 To importLastRow

for uniqueIndex = 2 to uniqueLastRow

For targetColumn = 0 To 7

But with the last one, you are using it as a column so you'd be better off adjusting the array than the column -

 For z = 1 To 8

    Out_Sheet.Cells(i + 2, z).Value = Imp_Arr(i, z - 1)

When you have a string like this -

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 3) = "MANUAL INPUT REQUIRED"

I find it better to get that out of the way up top

Const INPUT_REQUIRED As String = "MANUAL IMPORT REQUIRED"

You have a lot of comments. Comments - "code tell you how, comments tell you why". The code should speak for itself, if it needs a comment, it might need to be made more clear. If not, the comment should describe why you're doing something rather than how you're doing it. Here are a few reasons to avoid comments all together.

Writing to sheet

You have this chunk of code

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 0) = Imp_Sheet.Cells(i, 3)

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 1) = Imp_Sheet.Cells(i, 7)

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 2) = Imp_Sheet.Cells(i, 8)

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 3) = "MANUAL INPUT REQUIRED"

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 4) = Imp_Sheet.Cells(i, 6)

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 5) = Imp_Sheet.Cells(i, 4)

Imp_Arr(Unique_Sheet.Cells(x, 2) - 1, 6) = Imp_Sheet.Cells(i, 5)

That is pretty difficult to follow, yeah? Rearrange your array and write it all at once -

 Imp_Sheet.Range(.Cells(i, 3), .Cells(i, 8)) = Imp_Arr

Or pull out dimensions of the array into new arrays to put them on the sheet.

This If block doesn't need to be a block

If (Unique_Sheet.Cells(i, 1).Value = "") Then

  Exit For

End If

 If IsEmpty(Unique_Sheet.Cells(i, 1)) Then Exit For