PDA

View Full Version : Streamline code from MRecorder to Good Code Help



YellowLabPro
06-10-2006, 05:13 PM
Here is code from the macro recorder, starting on line 3


Lastrow = ActSh.Cells(Rows.Count, 1).End(xlUp).Row
ActSh.Range("A4", ActSh.Cells(Lastrow, "F")).ClearContents
Sheets("DataEdited").Select
Range("L2:S2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Copy
Sheets("PCCombined_FF").Select
Range("A4").Select
ActiveSheet.Paste
Cells.Columns.AutoFit


My thought is to convert it are
#1- Remove the .Select from Sheets("DataEdited").Select and Range("L2:S2").Select
#2- Remove Range(Selection, Selection.End(xlDown)).Select and replace w/ Select Up, (not that it matters in my current data, but thought it would be good practice
#3- Set a variable for the Sheet it needs to copy from and copy to, so Remove Range("A4").Select & ActiveSheet.Paste and replace w/ one statment.

My Attempt:

Dim rngSource As Range
Dim rngTarget As Range
Dim ActSh As Worksheet
Dim TrgSh As Worksheet
Dim Lastrow As Long
Set ActSh = Sheets("PCCombined_FF")
Set TrgShFF = Sheets("DataEdited")
Lastrow = ActSh.Cells(Rows.Count, 1).End(xlUp).Row
Lastrow = TrgShff.Range(Rows.Count,12).End(xlUp).Row
ActSh.Range("A4", ActSh.Cells(Lastrow, "F")).ClearContents

TrgSh.Range("L4",TrgSh.Cells(Lastrow, "S")).copy Destination:=Range("A4",ActSh.Range("A4"))

Thanks for having a look

YLP

malik641
06-10-2006, 05:45 PM
I would suggest to add the following:

Set ActSh = Nothing
Set Trgsh = Nothing But this doesn't seem like it works (at least in a test run it didn't work for me):

TrgSh.Range("L4", TrgSh.Cells(Lastrow, "S")).Copy Destination:=Range("A4", ActSh.Range("A4")) Although I think you wanted to do this:

TrgSh.Range("L4", TrgSh.Cells(Lastrow, "S")).Copy Destination:=ActSh.Range("A4")
Right???

YellowLabPro
06-10-2006, 05:52 PM
Hi Joseph,
I think you are correct in the last line of code. I have not tried your code, because it sounds like you were not able to get it to work based on my code, is that correct?

YLP

malik641
06-10-2006, 06:08 PM
Basically.

This is what I came up with...revised stuff in bold:
EDIT: I removed/changed some other things

Sub YLP()
Dim ActSh As Worksheet
Dim TrgSh As Worksheet
Dim Lastrow As Long

Set ActSh = Sheets("PCCombined_FF")
Set TrgSh = Sheets("DataEdited") 'Noticed this line didn't match your variable in your original code

Lastrow = ActSh.Cells(Rows.Count, 1).End(xlUp).Row
ActSh.Range("A4", ActSh.Cells(Lastrow, "F")).ClearContents

Lastrow = TrgSh.Cells(Rows.Count, 12).End(xlUp).Row
TrgSh.Range("L4", TrgSh.Cells(Lastrow, "S")).Copy Destination:=ActSh.Range("A4")

Set TrgSh = Nothing
Set ActSh = Nothing
End Sub
I wasn't sure what you were doing with "TrgShFF"....so the code above is just my interpretation of what you were trying to acclomplish.