Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/

Source Link

edited Apr 13, 2017 at 12:40

Finally, a precautionary note: You're using unqualified Sheets() which will reference the currently active workbook, whatever it may be. Since you indicate that this process is taking 15+ minutes, it's possible that someone may click on another Excel workbook or open a new one, and your processing will immediately start acting on whatever workbook now has the focus in Excel. Explicitly assigning a workbook/worksheet to a worksheet variable, as described by PeterT in his 3rd point his 3rd point will prevent your code from accidentally acting on a different workbook/worksheet.

Finally, a precautionary note: You're using unqualified Sheets() which will reference the currently active workbook, whatever it may be. Since you indicate that this process is taking 15+ minutes, it's possible that someone may click on another Excel workbook or open a new one, and your processing will immediately start acting on whatever workbook now has the focus in Excel. Explicitly assigning a workbook/worksheet to a worksheet variable, as described by PeterT in his 3rd point will prevent your code from accidentally acting on a different workbook/worksheet.

Source Link

answered Jun 21, 2016 at 15:07

FreeMan

1.3k
8
16

For simple readability, you can change this:

With Sheets("CDGL")
rows_c1 = .Cells(Rows.Count, "G").End(xlUp).Row
Sheets("Duplicate Check").Range("A1:C" & rows_c1).Value = Sheets("CDGL").Range("H2:J" & rows_c1).Value

rows_c2 = .Cells(Rows.Count, "K").End(xlUp).Row
Sheets("Duplicate Check").Range("D1:G" & rows_c2).Value = Sheets("CDGL").Range("L2:O" & rows_c2).Value

rows_c3 = .Cells(Rows.Count, "AI").End(xlUp).Row
Sheets("Duplicate Check").Range("H1:H" & rows_c3).Value = Sheets("CDGL").Range("AJ2:AJ" & rows_c3).Value
End With

to this:

With Sheets("CDGL")
  rows_c1 = .Cells(Rows.Count, "G").End(xlUp).Row
  Sheets("Duplicate Check").Range("A1:C" & rows_c1).Value = .Range("H2:J" & rows_c1).Value

  rows_c2 = .Cells(Rows.Count, "K").End(xlUp).Row
  Sheets("Duplicate Check").Range("D1:G" & rows_c2).Value = .Range("L2:O" & rows_c2).Value

  rows_c3 = .Cells(Rows.Count, "AI").End(xlUp).Row
  Sheets("Duplicate Check").Range("H1:H" & rows_c3).Value = .Range("AJ2:AJ" & rows_c3).Value
End With

Note the 2 changes:

Indention of the rows within the With block makes it much easier to find where the code block ends
Since you're using With Sheets("CDGL"), there's no need to specify Sheets("CDGL") on each of the assignment rows that follow. This shortens the rows and makes them easier to read.

I'm not sure about VBA processing speed on With statements, and since you're not looping over it, it may not make a huge difference, but simply removing the With block (like this):

rows_c1 = .Cells(Rows.Count, "G").End(xlUp).Row
Sheets("Duplicate Check").Range("A1:C" & rows_c1).Value = Sheets("CDGL").Range("H2:J" & rows_c1).Value

rows_c2 = .Cells(Rows.Count, "K").End(xlUp).Row
Sheets("Duplicate Check").Range("D1:G" & rows_c2).Value = Sheets("CDGL").Range("L2:O" & rows_c2).Value

rows_c3 = .Cells(Rows.Count, "AI").End(xlUp).Row
Sheets("Duplicate Check").Range("H1:H" & rows_c3).Value = Sheets("CDGL").Range("AJ2:AJ" & rows_c3).Value

might make a slight speed improvement. Since you'd already given the full Sheet specification, there was no need to include it in a With structure in the first place.

Finally, a precautionary note: You're using unqualified Sheets() which will reference the currently active workbook, whatever it may be. Since you indicate that this process is taking 15+ minutes, it's possible that someone may click on another Excel workbook or open a new one, and your processing will immediately start acting on whatever workbook now has the focus in Excel. Explicitly assigning a workbook/worksheet to a worksheet variable, as described by PeterT in his 3rd point will prevent your code from accidentally acting on a different workbook/worksheet.