4
\$\begingroup\$

Pretty new to this hence taking it as challenge. I have a master sheet in which Column A have all the file names( complete file name with .xls extensions). I need to copy data of column b in this sheet and paste it in all the files in a particular folder in a particular tab. There can be multiple items for one name. Sample Data

Australia.xlsx  XX
Austria.xlsx    YY
Austria.xlsx    DD
Belgium.xlsx    OO
Brazil.xlsx     MM
Brazil.xlsx     NN

Here is the code i tried to write:

Sub Macro1()
    Application.DisplayAlerts = False
    Dim Country As String
    Dim i As Integer
    Dim j As Integer
    Dim k As Integer

    k = 2
    For i = 2 To 50

        Windows("try 1 split to combines").Activate
        Country = Worksheets("Sheet3").Cells(i, 1).Value
        Workbooks("try 1 split to combines").Sheets("Sheet3").Range(Cells(i, i + 1)).Copy

        Workbooks.Open Filename:= _
        "C:\Users\abhayl.INDIA\Desktop\For Primary Team\For Primary Team\" & Country & " "
        'Workbooks("Name").Activate
        ActiveWorkbook.Sheets("Specific Questions").Activate
        ActiveWorkbook.Sheets("Specific Questions").Cells(k, 2).xlPasteValues
        finalrow2 = 42 + k
        k = finalrow2 + 1
        Workbooks(Country).Sheets("Specific Questions").Activate
        Workbooks(Country).Close SaveChanges:=False
    Next i
End Sub

This is pretty basic but need some improving, can anyone help me out to better rewrite this code?

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Your code and problem description is confusing. It seems as if you're halfway debugging the posted code. However, I've set up an example below that you can adapt to your situation. Some notable points:

  1. Always use Option Explicit
  2. Using Activate or Select is almost never necessary
  3. Use clear variables to indicate the source and destination data, it will help you keep things straight when writing your code and also later when you have to maintain it.
  4. An internal set of variables can be used to copy the data from one workbook to another. Using the Excel Copy is not necessary.
  5. The "clean up" code at the bottom of my example is not strictly necessary in VBA (the interpreter will detect those objects as out of scope and will destroy them automatically). But for me it's a good habit to keep track of what I'm using.
  6. Saving the previous state of the DisplayAlerts flag is a good idea when your code will change it. You don't necessarily know what the user's current setting will be and, aside from the changes your macro creates, you don't want the user to experience sudden unexpected changes.
Option Explicit

Sub Macro1()
    Dim previousAlertsFlag As Boolean
    Dim masterWB As Workbook
    Dim masterWS As Worksheet
    Dim destWB As Workbook
    Dim destWS As Worksheet
    Dim lastRow As Long
    Dim filepath As String
    Dim fullpath As String
    Dim country As String
    Dim countryData As Variant
    Dim r As Integer

    '--- save the current state of Alerts display, so it can
    '    be restored later
    previousAlertsFlag = Application.DisplayAlerts
    Application.DisplayAlerts = False

    '--- establish connections to the master data
    Set masterWB = ThisWorkbook
    Set masterWS = masterWB.Worksheets("Sheet3")
    lastRow = masterWS.Cells(masterWS.Rows.Count, "A").End(xlUp).Row
    filepath = "C:\Users\abhayl.INDIA\Desktop\For Primary Team\For Primary Team\"

    For r = 2 To lastRow
        country = masterWS.Cells(r, 1).Value
        countryData = masterWS.Cells(r, 2).Value

        fullpath = filepath & country
        Set destWB = Workbooks.Open(fullpath)
        Set destWS = destWB.Sheets("Specific Questions")
        destWS.Cells(1, 2).Value = countryData
        destWB.Close SaveChanges:=True
    Next i

    '--- clean up
    Set destWS = Nothing
    Set destWB = Nothing
    Set masterWB = Nothing
    Set masterWS = Nothing

    '--- restore the previous state of the Alerts flag
    Application.DisplayAlerts = previousAlertsFlag
End Sub
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Why not make filepath a constant? I like you storing the displayalerts state. I'm not sure you need the clean up. Good answer! \$\endgroup\$ Commented Jun 1, 2016 at 17:32
  • 1
    \$\begingroup\$ If the list of files is long, then look at using Application.ScreenUpdating to turn off the screen refresh which will make the macro run much faster. \$\endgroup\$ Commented Jun 2, 2016 at 7:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.