Opening a workbook and copying pages to master workbook
up vote
0
down vote
favorite
I have this code which opens 2 workbooks, copies a sheet and paste then to the master workbook. It is currently taking 3 minutes. Can this be done quicker (i.e. without opening each workbook to copy)?
It takes roughly 3 minutes to do.
Sub Load()
Dim masterWB As Workbook
Dim dailyWB As Workbook
Dim lastweekWB As Workbook
Dim R As Range
Dim B As Range
Dim C As Range
Dim Lrow As Long
Application.DisplayAlerts = False
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = Workbooks.Open(Sheets("Control Manager").Range("O2"))
'Copy the Range from dailyWB and Paste it into the MasterWB
dailyWB.Sheets("Summary1").Range("A1:BJ200").Copy masterWB.Sheets("Summary").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("Summary").Select
'trim values
Columns("A:BJ").Select
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for next Sheet
dailyWB.Sheets("risk1").Range("A1:BB200").Copy masterWB.Sheets("risk").Range("A1").Rows("1:1")
'formatting and paste as values'
masterWB.Activate
Worksheets("risk").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for CS sheet
dailyWB.Sheets("CS today").Range("A1:L3").Copy masterWB.Sheets("CS").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("CS").Select
Columns("A:L").Select
'trim cells to exclude spaces.
With Application.WorksheetFunction
For Each R In Intersect(Columns("A:L"), ActiveSheet.UsedRange)
R.Value = .Trim(R.Value)
Next R
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = Workbooks.Open(Sheets("Control Manager").Range("O3"))
'repeat for next risk Sheet
lastweekWB.Sheets("risk2").Range("A1:BB200").Copy masterWB.Sheets("risk_lastweek").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("risk_lastweek").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
Application.DisplayAlerts = True
'Close the Workbook without saving
dailyWB.Close False
lastweekWB.Close False
'Clear the Variables
Set dailyWB = Nothing
Set masterWB = Nothing
Set lastweekWB = Nothing
End Sub
vba
add a comment |
up vote
0
down vote
favorite
I have this code which opens 2 workbooks, copies a sheet and paste then to the master workbook. It is currently taking 3 minutes. Can this be done quicker (i.e. without opening each workbook to copy)?
It takes roughly 3 minutes to do.
Sub Load()
Dim masterWB As Workbook
Dim dailyWB As Workbook
Dim lastweekWB As Workbook
Dim R As Range
Dim B As Range
Dim C As Range
Dim Lrow As Long
Application.DisplayAlerts = False
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = Workbooks.Open(Sheets("Control Manager").Range("O2"))
'Copy the Range from dailyWB and Paste it into the MasterWB
dailyWB.Sheets("Summary1").Range("A1:BJ200").Copy masterWB.Sheets("Summary").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("Summary").Select
'trim values
Columns("A:BJ").Select
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for next Sheet
dailyWB.Sheets("risk1").Range("A1:BB200").Copy masterWB.Sheets("risk").Range("A1").Rows("1:1")
'formatting and paste as values'
masterWB.Activate
Worksheets("risk").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for CS sheet
dailyWB.Sheets("CS today").Range("A1:L3").Copy masterWB.Sheets("CS").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("CS").Select
Columns("A:L").Select
'trim cells to exclude spaces.
With Application.WorksheetFunction
For Each R In Intersect(Columns("A:L"), ActiveSheet.UsedRange)
R.Value = .Trim(R.Value)
Next R
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = Workbooks.Open(Sheets("Control Manager").Range("O3"))
'repeat for next risk Sheet
lastweekWB.Sheets("risk2").Range("A1:BB200").Copy masterWB.Sheets("risk_lastweek").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("risk_lastweek").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
Application.DisplayAlerts = True
'Close the Workbook without saving
dailyWB.Close False
lastweekWB.Close False
'Clear the Variables
Set dailyWB = Nothing
Set masterWB = Nothing
Set lastweekWB = Nothing
End Sub
vba
add a comment |
up vote
0
down vote
favorite
up vote
0
down vote
favorite
I have this code which opens 2 workbooks, copies a sheet and paste then to the master workbook. It is currently taking 3 minutes. Can this be done quicker (i.e. without opening each workbook to copy)?
It takes roughly 3 minutes to do.
Sub Load()
Dim masterWB As Workbook
Dim dailyWB As Workbook
Dim lastweekWB As Workbook
Dim R As Range
Dim B As Range
Dim C As Range
Dim Lrow As Long
Application.DisplayAlerts = False
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = Workbooks.Open(Sheets("Control Manager").Range("O2"))
'Copy the Range from dailyWB and Paste it into the MasterWB
dailyWB.Sheets("Summary1").Range("A1:BJ200").Copy masterWB.Sheets("Summary").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("Summary").Select
'trim values
Columns("A:BJ").Select
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for next Sheet
dailyWB.Sheets("risk1").Range("A1:BB200").Copy masterWB.Sheets("risk").Range("A1").Rows("1:1")
'formatting and paste as values'
masterWB.Activate
Worksheets("risk").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for CS sheet
dailyWB.Sheets("CS today").Range("A1:L3").Copy masterWB.Sheets("CS").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("CS").Select
Columns("A:L").Select
'trim cells to exclude spaces.
With Application.WorksheetFunction
For Each R In Intersect(Columns("A:L"), ActiveSheet.UsedRange)
R.Value = .Trim(R.Value)
Next R
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = Workbooks.Open(Sheets("Control Manager").Range("O3"))
'repeat for next risk Sheet
lastweekWB.Sheets("risk2").Range("A1:BB200").Copy masterWB.Sheets("risk_lastweek").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("risk_lastweek").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
Application.DisplayAlerts = True
'Close the Workbook without saving
dailyWB.Close False
lastweekWB.Close False
'Clear the Variables
Set dailyWB = Nothing
Set masterWB = Nothing
Set lastweekWB = Nothing
End Sub
vba
I have this code which opens 2 workbooks, copies a sheet and paste then to the master workbook. It is currently taking 3 minutes. Can this be done quicker (i.e. without opening each workbook to copy)?
It takes roughly 3 minutes to do.
Sub Load()
Dim masterWB As Workbook
Dim dailyWB As Workbook
Dim lastweekWB As Workbook
Dim R As Range
Dim B As Range
Dim C As Range
Dim Lrow As Long
Application.DisplayAlerts = False
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = Workbooks.Open(Sheets("Control Manager").Range("O2"))
'Copy the Range from dailyWB and Paste it into the MasterWB
dailyWB.Sheets("Summary1").Range("A1:BJ200").Copy masterWB.Sheets("Summary").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("Summary").Select
'trim values
Columns("A:BJ").Select
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for next Sheet
dailyWB.Sheets("risk1").Range("A1:BB200").Copy masterWB.Sheets("risk").Range("A1").Rows("1:1")
'formatting and paste as values'
masterWB.Activate
Worksheets("risk").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
'repeat for CS sheet
dailyWB.Sheets("CS today").Range("A1:L3").Copy masterWB.Sheets("CS").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("CS").Select
Columns("A:L").Select
'trim cells to exclude spaces.
With Application.WorksheetFunction
For Each R In Intersect(Columns("A:L"), ActiveSheet.UsedRange)
R.Value = .Trim(R.Value)
Next R
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = Workbooks.Open(Sheets("Control Manager").Range("O3"))
'repeat for next risk Sheet
lastweekWB.Sheets("risk2").Range("A1:BB200").Copy masterWB.Sheets("risk_lastweek").Range("A1").Rows("1:1")
'formatting and paste as values
masterWB.Activate
Worksheets("risk_lastweek").Select
Columns("A:BB").Select
With Application.WorksheetFunction
For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
B.Value = .Trim(B.Value)
Next B
End With
Selection.Columns.AutoFit
Selection.Copy
Selection.PasteSpecial xlPasteValues
Application.DisplayAlerts = True
'Close the Workbook without saving
dailyWB.Close False
lastweekWB.Close False
'Clear the Variables
Set dailyWB = Nothing
Set masterWB = Nothing
Set lastweekWB = Nothing
End Sub
vba
vba
edited Dec 13 at 2:58
Jamal♦
30.2k11116226
30.2k11116226
asked Dec 12 at 18:02
excelguy
447
447
add a comment |
add a comment |
1 Answer
1
active
oldest
votes
up vote
2
down vote
Separating tasks into multiple subroutines will make the code easier to test and modify.
This video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.
I would use .Range("A1")
instead of .Range("A1").Rows("1:1")
because Range.Copy
targets the first cell in the destination.
Refactored Code
Sub Load()
LoadDailyWorkbook
LoadLastWeeksWorkbook
End Sub
Sub LoadDailyWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Const A1L3 As String = "A1:L3"
Dim masterWB As Workbook
Dim dailyWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))
If Not dailyWB Is Nothing Then
With dailyWB
'Copy the Range from dailyWB and Paste it into the MasterWB
.Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
'repeat for next Sheet
.Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
TrimRange masterWB.Worksheets("risk").Range(A1BJ200)
'repeat for CS sheet
.Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
TrimRange masterWB.Worksheets("CS").Range(A1L3)
.Close SaveChanges:=False
End With
End If
End Sub
Sub LoadLastWeeksWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Dim masterWB As Workbook
Dim lastweekWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
If Not lastweekWB Is Nothing Then
With lastweekWB
'repeat for next risk Sheet
.Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)
TrimRange masterWB.Columns("A:BB")
.Close SaveChanges:=False
End With
End If
End Sub
Function getWorkbook(FullName As String) As Workbook
If Len(Dir(FullName)) = 0 Then
MsgBox FullName & " not found found", vbCritical, "File Not Found"
Else
Set getWorkbook = Workbooks.Open(FullName)
End If
End Function
Sub TrimRange(Target As Range)
Dim results As Variant
Set Target = Intersect(Target.Parent.UsedRange, Target)
If Target Is Nothing Then
Exit Sub
ElseIf Target.Count = 1 Then
Target.Value = Trim(Target.Value)
Exit Sub
Else
results = Target.Value
Dim r As Long, c As Long
For r = 1 To UBound(results)
For c = 1 To UBound(results, 2)
results(r, c) = Trim(results(r, c))
Next
Next
Target.Value = results
End If
Target.Columns.EntireColumn.AutoFit
End Sub
thanks for this, gettingtype mismatch
error onSet Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have beenTarget.Parent.UsedRange
.
– TinMan
Dec 12 at 21:21
overflow error now lol,Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I putElse If
instead ofElseIf
but it should be a syntax error. I don't know why you would get an overflow error.
– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
|
show 5 more comments
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209548%2fopening-a-workbook-and-copying-pages-to-master-workbook%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
Separating tasks into multiple subroutines will make the code easier to test and modify.
This video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.
I would use .Range("A1")
instead of .Range("A1").Rows("1:1")
because Range.Copy
targets the first cell in the destination.
Refactored Code
Sub Load()
LoadDailyWorkbook
LoadLastWeeksWorkbook
End Sub
Sub LoadDailyWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Const A1L3 As String = "A1:L3"
Dim masterWB As Workbook
Dim dailyWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))
If Not dailyWB Is Nothing Then
With dailyWB
'Copy the Range from dailyWB and Paste it into the MasterWB
.Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
'repeat for next Sheet
.Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
TrimRange masterWB.Worksheets("risk").Range(A1BJ200)
'repeat for CS sheet
.Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
TrimRange masterWB.Worksheets("CS").Range(A1L3)
.Close SaveChanges:=False
End With
End If
End Sub
Sub LoadLastWeeksWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Dim masterWB As Workbook
Dim lastweekWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
If Not lastweekWB Is Nothing Then
With lastweekWB
'repeat for next risk Sheet
.Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)
TrimRange masterWB.Columns("A:BB")
.Close SaveChanges:=False
End With
End If
End Sub
Function getWorkbook(FullName As String) As Workbook
If Len(Dir(FullName)) = 0 Then
MsgBox FullName & " not found found", vbCritical, "File Not Found"
Else
Set getWorkbook = Workbooks.Open(FullName)
End If
End Function
Sub TrimRange(Target As Range)
Dim results As Variant
Set Target = Intersect(Target.Parent.UsedRange, Target)
If Target Is Nothing Then
Exit Sub
ElseIf Target.Count = 1 Then
Target.Value = Trim(Target.Value)
Exit Sub
Else
results = Target.Value
Dim r As Long, c As Long
For r = 1 To UBound(results)
For c = 1 To UBound(results, 2)
results(r, c) = Trim(results(r, c))
Next
Next
Target.Value = results
End If
Target.Columns.EntireColumn.AutoFit
End Sub
thanks for this, gettingtype mismatch
error onSet Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have beenTarget.Parent.UsedRange
.
– TinMan
Dec 12 at 21:21
overflow error now lol,Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I putElse If
instead ofElseIf
but it should be a syntax error. I don't know why you would get an overflow error.
– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
|
show 5 more comments
up vote
2
down vote
Separating tasks into multiple subroutines will make the code easier to test and modify.
This video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.
I would use .Range("A1")
instead of .Range("A1").Rows("1:1")
because Range.Copy
targets the first cell in the destination.
Refactored Code
Sub Load()
LoadDailyWorkbook
LoadLastWeeksWorkbook
End Sub
Sub LoadDailyWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Const A1L3 As String = "A1:L3"
Dim masterWB As Workbook
Dim dailyWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))
If Not dailyWB Is Nothing Then
With dailyWB
'Copy the Range from dailyWB and Paste it into the MasterWB
.Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
'repeat for next Sheet
.Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
TrimRange masterWB.Worksheets("risk").Range(A1BJ200)
'repeat for CS sheet
.Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
TrimRange masterWB.Worksheets("CS").Range(A1L3)
.Close SaveChanges:=False
End With
End If
End Sub
Sub LoadLastWeeksWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Dim masterWB As Workbook
Dim lastweekWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
If Not lastweekWB Is Nothing Then
With lastweekWB
'repeat for next risk Sheet
.Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)
TrimRange masterWB.Columns("A:BB")
.Close SaveChanges:=False
End With
End If
End Sub
Function getWorkbook(FullName As String) As Workbook
If Len(Dir(FullName)) = 0 Then
MsgBox FullName & " not found found", vbCritical, "File Not Found"
Else
Set getWorkbook = Workbooks.Open(FullName)
End If
End Function
Sub TrimRange(Target As Range)
Dim results As Variant
Set Target = Intersect(Target.Parent.UsedRange, Target)
If Target Is Nothing Then
Exit Sub
ElseIf Target.Count = 1 Then
Target.Value = Trim(Target.Value)
Exit Sub
Else
results = Target.Value
Dim r As Long, c As Long
For r = 1 To UBound(results)
For c = 1 To UBound(results, 2)
results(r, c) = Trim(results(r, c))
Next
Next
Target.Value = results
End If
Target.Columns.EntireColumn.AutoFit
End Sub
thanks for this, gettingtype mismatch
error onSet Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have beenTarget.Parent.UsedRange
.
– TinMan
Dec 12 at 21:21
overflow error now lol,Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I putElse If
instead ofElseIf
but it should be a syntax error. I don't know why you would get an overflow error.
– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
|
show 5 more comments
up vote
2
down vote
up vote
2
down vote
Separating tasks into multiple subroutines will make the code easier to test and modify.
This video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.
I would use .Range("A1")
instead of .Range("A1").Rows("1:1")
because Range.Copy
targets the first cell in the destination.
Refactored Code
Sub Load()
LoadDailyWorkbook
LoadLastWeeksWorkbook
End Sub
Sub LoadDailyWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Const A1L3 As String = "A1:L3"
Dim masterWB As Workbook
Dim dailyWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))
If Not dailyWB Is Nothing Then
With dailyWB
'Copy the Range from dailyWB and Paste it into the MasterWB
.Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
'repeat for next Sheet
.Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
TrimRange masterWB.Worksheets("risk").Range(A1BJ200)
'repeat for CS sheet
.Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
TrimRange masterWB.Worksheets("CS").Range(A1L3)
.Close SaveChanges:=False
End With
End If
End Sub
Sub LoadLastWeeksWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Dim masterWB As Workbook
Dim lastweekWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
If Not lastweekWB Is Nothing Then
With lastweekWB
'repeat for next risk Sheet
.Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)
TrimRange masterWB.Columns("A:BB")
.Close SaveChanges:=False
End With
End If
End Sub
Function getWorkbook(FullName As String) As Workbook
If Len(Dir(FullName)) = 0 Then
MsgBox FullName & " not found found", vbCritical, "File Not Found"
Else
Set getWorkbook = Workbooks.Open(FullName)
End If
End Function
Sub TrimRange(Target As Range)
Dim results As Variant
Set Target = Intersect(Target.Parent.UsedRange, Target)
If Target Is Nothing Then
Exit Sub
ElseIf Target.Count = 1 Then
Target.Value = Trim(Target.Value)
Exit Sub
Else
results = Target.Value
Dim r As Long, c As Long
For r = 1 To UBound(results)
For c = 1 To UBound(results, 2)
results(r, c) = Trim(results(r, c))
Next
Next
Target.Value = results
End If
Target.Columns.EntireColumn.AutoFit
End Sub
Separating tasks into multiple subroutines will make the code easier to test and modify.
This video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.
I would use .Range("A1")
instead of .Range("A1").Rows("1:1")
because Range.Copy
targets the first cell in the destination.
Refactored Code
Sub Load()
LoadDailyWorkbook
LoadLastWeeksWorkbook
End Sub
Sub LoadDailyWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Const A1L3 As String = "A1:L3"
Dim masterWB As Workbook
Dim dailyWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
'Set some Workbook as the one you are copying from
Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))
If Not dailyWB Is Nothing Then
With dailyWB
'Copy the Range from dailyWB and Paste it into the MasterWB
.Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
'repeat for next Sheet
.Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
TrimRange masterWB.Worksheets("risk").Range(A1BJ200)
'repeat for CS sheet
.Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
TrimRange masterWB.Worksheets("CS").Range(A1L3)
.Close SaveChanges:=False
End With
End If
End Sub
Sub LoadLastWeeksWorkbook()
Const A1BJ200 As String = "A1:BJ200"
Dim masterWB As Workbook
Dim lastweekWB As Workbook
'Set Current Workbook as Master
Set masterWB = Application.ThisWorkbook
''''''''''''Get Last Week Data''''''''''''''''''''''
Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
If Not lastweekWB Is Nothing Then
With lastweekWB
'repeat for next risk Sheet
.Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)
TrimRange masterWB.Columns("A:BB")
.Close SaveChanges:=False
End With
End If
End Sub
Function getWorkbook(FullName As String) As Workbook
If Len(Dir(FullName)) = 0 Then
MsgBox FullName & " not found found", vbCritical, "File Not Found"
Else
Set getWorkbook = Workbooks.Open(FullName)
End If
End Function
Sub TrimRange(Target As Range)
Dim results As Variant
Set Target = Intersect(Target.Parent.UsedRange, Target)
If Target Is Nothing Then
Exit Sub
ElseIf Target.Count = 1 Then
Target.Value = Trim(Target.Value)
Exit Sub
Else
results = Target.Value
Dim r As Long, c As Long
For r = 1 To UBound(results)
For c = 1 To UBound(results, 2)
results(r, c) = Trim(results(r, c))
Next
Next
Target.Value = results
End If
Target.Columns.EntireColumn.AutoFit
End Sub
edited Dec 15 at 7:43
answered Dec 12 at 20:39
TinMan
1,027110
1,027110
thanks for this, gettingtype mismatch
error onSet Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have beenTarget.Parent.UsedRange
.
– TinMan
Dec 12 at 21:21
overflow error now lol,Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I putElse If
instead ofElseIf
but it should be a syntax error. I don't know why you would get an overflow error.
– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
|
show 5 more comments
thanks for this, gettingtype mismatch
error onSet Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have beenTarget.Parent.UsedRange
.
– TinMan
Dec 12 at 21:21
overflow error now lol,Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I putElse If
instead ofElseIf
but it should be a syntax error. I don't know why you would get an overflow error.
– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
thanks for this, getting
type mismatch
error on Set Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
thanks for this, getting
type mismatch
error on Set Target = Intersect(Target.Parent, Target)
– excelguy
Dec 12 at 21:17
@excelguy I updated the code. Should have been
Target.Parent.UsedRange
.– TinMan
Dec 12 at 21:21
@excelguy I updated the code. Should have been
Target.Parent.UsedRange
.– TinMan
Dec 12 at 21:21
overflow error now lol,
Target = Target.Value
– excelguy
Dec 12 at 21:41
overflow error now lol,
Target = Target.Value
– excelguy
Dec 12 at 21:41
Hmmm....I've been writing javascript so I put
Else If
instead of ElseIf
but it should be a syntax error. I don't know why you would get an overflow error.– TinMan
Dec 12 at 23:28
Hmmm....I've been writing javascript so I put
Else If
instead of ElseIf
but it should be a syntax error. I don't know why you would get an overflow error.– TinMan
Dec 12 at 23:28
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
Not sure either, ill look into it.
– excelguy
Dec 13 at 14:06
|
show 5 more comments
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209548%2fopening-a-workbook-and-copying-pages-to-master-workbook%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown