2 Part question: Using Like & refining code

[spydey]

Right off the bat, if this should be broken out into 2 separate threads, I apologize and will adjust accordingly (if possible). I just see the two issues somewhat related and so thought that perhaps I could get them both done in one swing.

Part 1:

The two workbooks I am using are:

WorkbookA & WorkbookB

WorkbookA contains the code in a module.
WorkbookA only has one sheet, titled/named "Summary".

WorkbookB is referenced in WorkbookA's code, as SourceFile.
Thus SourceFile = WookbookB's full path & file name, e.g. C:\Location\Of\Excel\Files\WorkbookB.xlsx
WorkbookB has multiple sheets, anywhere from 2 - 100+
The very first sheet of WorkbookB is titled/named "Summary - X","X" being the number of sheets present in WorkbookB, not counting Summary.
e.g. "Summary - 102"
"X" can change as sheets are added/removed from WorkbookB.



The purpose of the code is to copy specific cell values from the various sheets in WorkbookB over to specific columns in the "Summary" sheet in WorkbookA. However, I do not want to copy anything from the "Summary - X" sheet in WorkbookB.

I figured I could use Like with a wildcard, but I think I have my logic a bit backwards.

Here is the code:

Private Sub CopyData()


Dim ws As Worksheet
Dim LR1 As Long


Application.ScreenUpdating = False


Workbooks.Open filename:=SourceFile, ReadOnly:=True


For Each ws In ActiveWorkbook.Worksheets
    If ws.Name <> "Summary" Then
        LR1 = ThisWorkbook.Sheets("Summary").Range("A" & Rows.Count).End(xlUp).Row + 1
      With ws
        .Range("B2").Copy
        ThisWorkbook.Sheets("Summary").Range("A" & LR1).PasteSpecial xlValues
        .Range("B6").Copy
        ThisWorkbook.Sheets("Summary").Range("B" & LR1).PasteSpecial xlValues
        .Range("B12").Copy
        ThisWorkbook.Sheets("Summary").Range("E" & LR1).PasteSpecial xlValues
        .Range("B13").Copy
        ThisWorkbook.Sheets("Summary").Range("C" & LR1).PasteSpecial xlValues
        .Range("B4").Copy
        ThisWorkbook.Sheets("Summary").Range("D" & LR1).PasteSpecial xlValues
        .Range("B5").Copy
        ThisWorkbook.Sheets("Summary").Range("F" & LR1).PasteSpecial xlValues
        .Range("B9").Copy
        ThisWorkbook.Sheets("Summary").Range("G" & LR1).PasteSpecial xlValues
        .Range("G3").Copy
        ThisWorkbook.Sheets("Summary").Range("H" & LR1).PasteSpecial xlValues
        .Range("G4").Copy
        ThisWorkbook.Sheets("Summary").Range("J" & LR1).PasteSpecial xlValues
        .Range("G6").Copy
        ThisWorkbook.Sheets("Summary").Range("I" & LR1).PasteSpecial xlValues
        .Range("J4").Copy
        ThisWorkbook.Sheets("Summary").Range("K" & LR1).PasteSpecial xlValues
        .Range("J5").Copy
        ThisWorkbook.Sheets("Summary").Range("L" & LR1).PasteSpecial xlValues
        .Range("J6").Copy
        ThisWorkbook.Sheets("Summary").Range("M" & LR1).PasteSpecial xlValues
        .Range("J7").Copy
        ThisWorkbook.Sheets("Summary").Range("N" & LR1).PasteSpecial xlValues
        .Range("J8").Copy
        ThisWorkbook.Sheets("Summary").Range("O" & LR1).PasteSpecial xlValues
        .Range("J9").Copy
        ThisWorkbook.Sheets("Summary").Range("P" & LR1).PasteSpecial xlValues
        .Range("J10").Copy
        ThisWorkbook.Sheets("Summary").Range("Q" & LR1).PasteSpecial xlValues
        .Range("J11").Copy
        ThisWorkbook.Sheets("Summary").Range("R" & LR1).PasteSpecial xlValues
        .Range("J12").Copy
        ThisWorkbook.Sheets("Summary").Range("S" & LR1).PasteSpecial xlValues
                
      End With
    End If
Next ws
 
ActiveWorkbook.Close (False)
 
End Sub

I need to exclude the "Summary - X" sheet data from being copied, but can not reference it as Summary because the name can continually change due to how many sheets there are.

Any thoughts?

Part 2:

How can I go about cleaning up my code? Is there a simpler way to achieve what I am aiming for over all? I want to make it more efficient if possible.

Thanks!!

-Spydey

 

[Fluff]

First off try

If Left(Ws.Name, 7) <> "Summary" Then

 

[Fluff]

For part 2, depends on preference, but I'd tend to right it like this

Private Sub CopyData()


Dim ws As Worksheet
Dim LR1 As Long
Dim Wbk As Workbook

Application.ScreenUpdating = False


Set Wbk = Workbooks.Open(Filename:=SourceFile, ReadOnly:=True)


For Each ws In Wbk.Worksheets
    If Left(ws.Name, 7) <> "Summary" Then
      LR1 = ThisWorkbook.Sheets("Summary").Range("A" & Rows.Count).End(xlUp).Row + 1
      With ThisWorkbook.Sheets("Summary")
        .Range("A" & LR1).Value = ws.Range("B2").Value
        .Range("B" & LR1).Value = ws.Range("B6").Value
        .Range("C" & LR1).Value = ws.Range("B13").Value
        .Range("D" & LR1).Value = ws.Range("B4").Value
        .Range("E" & LR1).Value = ws.Range("B12").Value
        .Range("F" & LR1).Value = ws.Range("B5").Value
        .Range("G" & LR1).Value = ws.Range("B9").Value
      End With
    End If
Next ws
 
Wbk.Close False
 
End Sub

 

[spydey]

First off try

If Left(Ws.Name, 7) <> "Summary" Then

Fantastic!! Works as I was looking for. I hadn't even thought about using the first 7 characters from the left of the Sheet name. Sometimes the answer can be so simple it escapes me ... Thank you so very much Fluff (once again!) for your time and assistance.

-Spydey

 

[spydey]

For part 2, depends on preference, but I'd tend to right it like this

Private Sub CopyData()


Dim ws As Worksheet
Dim LR1 As Long
Dim Wbk As Workbook

Application.ScreenUpdating = False


Set Wbk = Workbooks.Open(Filename:=SourceFile, ReadOnly:=True)


For Each ws In Wbk.Worksheets
    If Left(ws.Name, 7) <> "Summary" Then
      LR1 = ThisWorkbook.Sheets("Summary").Range("A" & Rows.Count).End(xlUp).Row + 1
      With ThisWorkbook.Sheets("Summary")
        .Range("A" & LR1).Value = ws.Range("B2").Value
        .Range("B" & LR1).Value = ws.Range("B6").Value
        .Range("C" & LR1).Value = ws.Range("B13").Value
        .Range("D" & LR1).Value = ws.Range("B4").Value
        .Range("E" & LR1).Value = ws.Range("B12").Value
        .Range("F" & LR1).Value = ws.Range("B5").Value
        .Range("G" & LR1).Value = ws.Range("B9").Value
      End With
    End If
Next ws
 
Wbk.Close False
 
End Sub

Ok, so I see what you did. It is much more compact, you set a few items, such as Wbk. I imagine so as to avoid having to type out longer strings, as well as to differentiate between the actual SourceFile and any other workbooks that might become the activeworkbook, right?

How do you go about recognizing what can be compacted, changed, corrected, etc, to come up with good efficient code? Does it just take experience, practice, etc? Or is there something else?

I would love to be able to look at something and see where it can be made to be more efficient.

Thank you Fluff for your help and willingness to assist.

-Spydey

 

[Fluff]

How do you go about recognizing what can be compacted, changed, corrected, etc, to come up with good efficient code? Does it just take experience, practice, etc? Or is there something else?

Experience mainly.

I added the Wbk variable, to ensure that the code is always looking in the right place.
When writing a macro for myself, I always avoid using anything that starts with Active as it can cause major problems. Especially when testing.
I swapped the With statement round, because I'm lazy & would prefer to type ws ~20 times rather than ThisWorkbook.Sheets("Summary")

Finally using .value=.value avoids using the clipboard.