patternMinor
VBA Copy Cells Stable
Viewed 0 times
cellsstablecopyvba
Problem
I'm very new to VBA and have put together a macro that currently does exactly what I want: copy cells from one worksheet to another, clear the cells copied, refresh my pivot tables and save the file.
Here is what I am using for it:
What I want to know is whether this macro will be stable as I continue to add lines of data to it in the future. Essentially I want to minimize the risk of the file crashing or my data being erased. Otherwise, would anyone have any suggestions to offer me to improve this? Thank you.
Here is what I am using for it:
Sub Copyinfo()
On Error GoTo Err_Execute
Sheet1.Range("A17:R17").Copy
Sheet2.Range("A2").Rows("2:2").Insert shift:=xlDown
Sheet2.Range("A2").Rows("2:2").PasteSpecial Paste:=xlPasteValuesAndNumberFormats
Sheet1.Range("C5:I16, Q5:R5").ClearContents
Err_Execute:
If Err.Number = 0 Then MsgBox "Copied Data", vbInformation Else _
MsgBox Err.Description
ThisWorkbook.RefreshAll
ThisWorkbook.Save
End SubWhat I want to know is whether this macro will be stable as I continue to add lines of data to it in the future. Essentially I want to minimize the risk of the file crashing or my data being erased. Otherwise, would anyone have any suggestions to offer me to improve this? Thank you.
Solution
few things that come to mind on a quick look:
You might consider naming this something other than "Copyinfo". If it is archiving transactions, for example, you might name it "ArchiveTransactions". If you do decide to stick with the name, you should go with the Pascal Case version: "CopyInfo". While you're at it: it's always good to specify public instead of leaving it implicit.
You don't need to do
As for the syntax with the
If you use
You might consider naming this something other than "Copyinfo". If it is archiving transactions, for example, you might name it "ArchiveTransactions". If you do decide to stick with the name, you should go with the Pascal Case version: "CopyInfo". While you're at it: it's always good to specify public instead of leaving it implicit.
You don't need to do
Sheet2.Range("A2").Rows("2:2"), you can also do Sheet2.Range("2:2") or just Sheet2.Rows("2:2"), or (and most simply) Sheet2.Rows(2). I like putting things in With blocks (although many people won't for just two things) because I like reducing code duplication.As for the syntax with the
If statement... If there is an Else, it should be on multiple lines, in my opinion. The way you had it above with the line continuation seems to go out of it's way to stay on one line (which, it doesn't anyway!). Clarity always wins over conciseness on my book.If you use
Application.ScreenUpdating as shown below, it will eliminate the screen flicker that can happen when you switch sheets, as well as speed up the performance (which, since this Sub doesn't do a whole lot, performance tuning isn't necessary).Public Sub Copyinfo()
Application.ScreenUpdating = False
On Error GoTo Err_Execute
Sheet1.Range("A17:R17").Copy
With Sheet2.Rows(2)
.Insert shift:=xlDown
.PasteSpecial Paste:=xlPasteValuesAndNumberFormats
End With
Sheet1.Range("C5:I16, Q5:R5").ClearContents
Err_Execute:
If Err.Number = 0 Then
MsgBox "Copied Data", vbInformation
Else
MsgBox Err.Description
End If
ThisWorkbook.RefreshAll
ThisWorkbook.Save
Application.ScreenUpdating = True
End SubCode Snippets
Public Sub Copyinfo()
Application.ScreenUpdating = False
On Error GoTo Err_Execute
Sheet1.Range("A17:R17").Copy
With Sheet2.Rows(2)
.Insert shift:=xlDown
.PasteSpecial Paste:=xlPasteValuesAndNumberFormats
End With
Sheet1.Range("C5:I16, Q5:R5").ClearContents
Err_Execute:
If Err.Number = 0 Then
MsgBox "Copied Data", vbInformation
Else
MsgBox Err.Description
End If
ThisWorkbook.RefreshAll
ThisWorkbook.Save
Application.ScreenUpdating = True
End SubContext
StackExchange Code Review Q#124163, answer score: 5
Revisions (0)
No revisions yet.