Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Variables

Variables

###Writing to sheet

Writing to sheet

###Variables

###Writing to sheet

Variables

Writing to sheet

Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60

###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
lang-vb

AltStyle によって変換されたページ (->オリジナル) /