Code review for efficiency (10 lines)

  • Thread starter Thread starter Tom
  • Start date Start date
T

Tom

Please could someone look at this code. Is there a more versatile, efficient
way of doing this? How do I use .codename instead of .name - I couldn't get
this to work.

I call the function using :
Call DoCopy(Sheet13.Range("AD2").Address, Sheet3.Range("D12").Address,
Sheet3.Name)

Public Function DoCopy(copyFromRange As String, pasteIntoRange As String,
pasteIntoSheet As String)

Range(copyFromRange).Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Copy

Worksheets(pasteIntoSheet).Activate
Range(pasteIntoRange).Select
ActiveSheet.Paste

End Function

Thanks

Tom
 
Tom

Try this:

Sub test()
Call DoCopy(Sheet13.Range("AD2").Address, _
Sheet3.Range("D12").Address, _
Sheet3.Name)
End Sub

Public Function DoCopy(copyFromRange As String, _
pasteIntoRange As String, _
pasteIntoSheet As String)

Range(Range(copyFromRange), _
Range(copyFromRange).End(xlDown)).Copy _
Worksheets(pasteIntoSheet).Range(pasteIntoRange)
End Function

Regards

Trevor
 
I think I'd be a little more careful with that range.address.

This line could cause trouble:
Range(copyFromRange).Select
You're passing a string, so it would look like:
Range("AD2").select

This will usually refer to the activesheet--and that may not be what you want to
use.

You could pass it the address and include the worksheet:

Call doCopy(sheet13.range("ad2").address(external:=true), .....

But even better you could just pass it the range object itself. That range
object has a parent--the worksheet. You don't even have to care!

Option Explicit
Sub test()
Call DoCopy(Sheet1.Range("AD2"), _
Sheet2.Range("D12"))
End Sub
Sub DoCopy(copyFromRange As Range, pasteIntoRange As Range)
Range(copyFromRange, copyFromRange.End(xlDown)).Copy _
Destination:=pasteIntoRange
End Sub

I think I'd have used a Subroutine instead of a Function.

And unless you know your data very well, you may want to include some error
checking. The first time I ran it, I didn't have anything in column AD and
..end(xldown) took me all the way to AD65536.

That caused an error.
 
Dave,

Thanks for this, it made a lot of sense, as you can tell I'm new to VB

Just a couple of points I need clarification on.. Are you using a sub not a
function because no value is returned, so strictly it should be a sub, or is
there any other reason that I'm not aware of?

Regarding the data, I do know this well, and in fact I can change this to as
below:
Range(copyFromRange, copyFromRange.Offset(20, 0)).Copy
pasteIntoRange.PasteSpecial _(xlPasteValues, xlPasteSpecialOperationNone,
False, False)

However that this code does not work, " the code can't get the pastespecial
property of the range class"? I have tried splitting it onto two lines, a
Range().copy and a Range().pastespecial, but I couldn't get that to work
either.

any thoughts?

Tom
 
If you decide to use a function, make it return a boolean value. Then you could
inspect that value to see if the copy worked:

if docopy(....) = false then
'some type of error handler
else
'continue with your process.
end if

I'm not sure what error you're getting, but this seemed to work ok for me:

Option Explicit
Sub doCopy(copyFromRange As Range, pasteIntoRange As Range)

Range(copyFromRange, copyFromRange.Offset(20, 0)).Copy
pasteIntoRange.PasteSpecial _
Paste:=xlPasteValues, operation:=xlPasteSpecialOperationNone, _
skipblanks:=False, Transpose:=False

End Sub

And if you're just pasting values--no formatting, nothing else, you can just
assign the value:

(this time using a function:

Option Explicit
Function doCopy(copyFromRange As Range, pasteIntoRange As Range) As Boolean

Dim RealRange As Range

Set RealRange = Nothing
On Error Resume Next
Set RealRange = Range(copyFromRange, copyFromRange.Offset(20, 0))
On Error GoTo 0

If RealRange Is Nothing Then
doCopy = False
Else
With RealRange
pasteIntoRange.Resize(.Rows.Count, .Columns.Count).Value _
= RealRange.Value
End With
doCopy = True
End If

End Function

Sub testme()
With ActiveSheet
MsgBox doCopy(.Range("a1"), .Range("b3"))
End With
End Sub

And you could add some other checks to see if the values got assigned
correctly--if the worksheet is protected, it could fail.
 
Back
Top