HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Copying a range from one file to another

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filerangeonecopyinganotherfrom

Problem

Since it has too many loop, loop inside loop, this works very slowly. How can we increase the speed of this program? I am trying to copy a range from one file to another file of same name in different folder. I have folders named "A","B","C"..."G" inside c:\Charts\1\ and c:\Charts\0\. Inside each folder "A","B","C"..."G" there are files named 1,2,3,4,..10.

dim vArr1 as array
Dim vArr as array
vArr1 = Array("A", "B", "C", "D", "E", "F", "G")
vArr = Array("A", "B", "C", "D", "E", "F", "G")
Dim fileName1, Pathname1 As String
Pathname1 = "c:\Charts\1\"
Pathname="c:\charts\0\"
For Each vFile1 In vArr1
    fileName1 = Dir(Pathname1 & vFile1 & "\" & "*.xlsx")
    Do While fileName1 <> ""
        For Each vFile In vArr
            filename = Dir(Pathname & vFile & "\" & "*.xlsx")
            Do While filename <> ""
                If filename = fileName1 Then
                Set WB1 = Workbooks.Open(Pathname1 & vFile & "\" & fileName1)
                    WB1.Application.ScreenUpdating = False
                    WB1.ActiveSheet.Range("M1:M30").Copy
                    WB1.Close (False)
                Set WBD1 = Workbooks.Open(Pathname & vFile & "\" & filename)
                WBD1.ActiveSheet.Range("C1").Select
                    WBD1.ActiveSheet.Paste
                    WBD1.ActiveSheet.Cells(1, 3).Value = "HSI Q4 2014-15"
                    WBD1.Close (True)
                    filename = Dir()
                Else

                End If
                fileName1 = filename
            Loop
        Next
    Loop
Next

Solution

I doubt this is how you've actually declared arrays:

dim vArr1 as array
Dim vArr as array


That would be

Dim vArr1() As String
Dim vArr() As String


But nevermind that: you need to use more meaningful names. If they're folder names, consider calling them something like folders or folderNames - and avoid arbitrary Hungarian-like prefixes such as v.

This is why multiple declarations in the same instruction should be avoided:

Dim fileName1, Pathname1 As String


Do you think fileName1 is a String? It's not. It's implicitly declared as Variant, because As String only applies to Pathname1!

Indentation is off here, and only looks the way it does because WB1 has 3 characters (and it's another bad name):

Set WB1 = Workbooks.Open(Pathname1 & vFile & "\" & fileName1)
    WB1.Application.ScreenUpdating = False
    WB1.ActiveSheet.Range("M1:M30").Copy
    WB1.Close (False)


Should be:

Set WB1 = Workbooks.Open(Pathname1 & vFile & "\" & fileName1)
WB1.Application.ScreenUpdating = False
WB1.ActiveSheet.Range("M1:M30").Copy
WB1.Close (False)


Note that your usage of parenthesis in method calls is going to cause a bug one day or another, when you pass an argument like that into a ByRef parameter.

How so? Well, the "normal" VBA syntax for passing arguments is like this:

WB1.Close False


By doing this:

WB1.Close (False)


You are forcing the argument to be passed ByVal, overriding the procedure's signature that might have said ByRef. Of course it doesn't really matter here for a Boolean literal..

Workbooks.Open(Pathname1 & vFile & "\" & fileName1)


...or even for a String literal. But one day you're going to want to pass an argument into a ByRef parameter, and learn the hard way that parentheses should be dropped when doing procedure calls. They're only needed for function calls.

The main reason why your code is slow is because you're opening and closing workbooks - there's not really anything to do to speed that up unfortunately.

Except perhaps...

WB1.Application.ScreenUpdating = False


Take that part out of the loop, and turn off screen updating all the while.

Application.ScreenUpdating = False


Now, whenever you turn off screen updating, you need to turn it back on when you're done, and you need to handle runtime errors and make sure you're turning it back on even when an error breaks you out of the code. You're accessing the file system here, so you have to handle runtime errors.

See this post for how to properly handle errors in VBA.

Code Snippets

dim vArr1 as array
Dim vArr as array
Dim vArr1() As String
Dim vArr() As String
Dim fileName1, Pathname1 As String
Set WB1 = Workbooks.Open(Pathname1 & vFile & "\" & fileName1)
    WB1.Application.ScreenUpdating = False
    WB1.ActiveSheet.Range("M1:M30").Copy
    WB1.Close (False)
Set WB1 = Workbooks.Open(Pathname1 & vFile & "\" & fileName1)
WB1.Application.ScreenUpdating = False
WB1.ActiveSheet.Range("M1:M30").Copy
WB1.Close (False)

Context

StackExchange Code Review Q#94550, answer score: 8

Revisions (0)

No revisions yet.