0

I am trying to copy a few colums of data that meet a certain criteria and then paste the first column of the copied data into a specific column on a second spreadsheet by nation. I am stuck selecting data from the copied cells- the second if statement.

New Working Code

Sub SortData()

'Clear Data from Practices Sheet
    Sheet2.Range("B6:F1000").Clear



a = Worksheets("Home").Cells(Rows.Count, 3).End(xlUp).Row

For i = 3 To a

    If Worksheets("Home").Cells(i, 4).Value = "Active" And Worksheets("Home").Cells(i, 3).Value = "Denmark" Then

    C = Worksheets("Home").Cells(i, 2).Copy

    Worksheets("Practices").Activate

    b = Worksheets("Practices").Cells(Rows.Count, 2).End(xlUp).Row

    Worksheets("Practices").Cells(b + 1, 2).Select 'column To paste data into

    ActiveSheet.Paste

    Worksheets("Home").Activate

    ElseIf Worksheets("Home").Cells(i, 4).Value = "Active" And Worksheets("Home").Cells(i, 3).Value = "Netherlands" Then

    C = Worksheets("Home").Cells(i, 2).Copy

    Worksheets("Practices").Activate

    b1 = Worksheets("Practices").Cells(Rows.Count, 4).End(xlUp).Row

    Worksheets("Practices").Cells(b1 + 1, 4).Select

    ActiveSheet.Paste

    Worksheets("Home").Activate

    ElseIf Worksheets("Home").Cells(i, 4).Value = "Active" And Worksheets("Home").Cells(i, 3).Value = "UK" Then

    C = Worksheets("Home").Cells(i, 2).Copy

    Worksheets("Practices").Activate

    b = Worksheets("Practices").Cells(Rows.Count, 6).End(xlUp).Row

    Worksheets("Practices").Cells(b + 1, 6).Select

    ActiveSheet.Paste

    Worksheets("Home").Activate
    End If

Next

End Sub

How to make this more concise?

Community
  • 1
  • 1
Infernez
  • 73
  • 7
  • To copy data you don't need to `.Activate` or `.Select` anything. Please read [How to avoid using Select in Excel VBA](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba). Also use `Option Explicit` and declare all your variables (at least `CopiedRow` is nothing!). Also this `If C.Cells( = "Denmark" Then` is no valid syntax. Make sure you have no compile errors **at first**! – Pᴇʜ May 14 '18 at 09:31
  • @Pᴇʜ, Ok. Noted. However My code trips out before that. I want to copy the rows and the columns, as stated in the first if statement then paste the particular data from the other if statements. Can you help with this? It is a typo! – Infernez May 14 '18 at 09:34
  • Well not using `.Select` and `.Activate` should be done first as this fixes many issues already. Also please [edit] your questions and tell which errors you get and in which line. – Pᴇʜ May 14 '18 at 09:35
  • @Pᴇʜ, Error 13 type mismatch for defining C – Infernez May 14 '18 at 09:37
  • ① `Cells(i, "1;4")` is no valid syntax. ② you cannot save a copy in a variable. • First thing you must do to get rid of your errors: Use `Option Explicit` and declare all your variables properly. Next avoid `.Select` and `.Activate`, then go through your code and see what errors you get and where. Use descriptive variable names instead of one character names. – Pᴇʜ May 14 '18 at 09:46

2 Answers2

1

I recommend to reduce redundant code like this:

  • Don't use .Select and .Activate as I told in my first comment.
    How to avoid using Select in Excel VBA
  • Use Option Explicit to make sure all variables are declared.
  • Don't use the same code lines over and over. Instead make a function/procedure or reduce redundancy like I did below.
  • Always use descriptive variable names instead of one letter names. Otherwise your code is very hard to read/understand by humans.

Option Explicit

Public Sub SortData()
    'Clear Data from Practices Sheet
    Worksheets("Practices").Range("B6:F1000").Clear

    Dim LastUsedRow As Long
    LastUsedRow = Worksheets("Home").Cells(Rows.Count, 3).End(xlUp).Row

    Dim i As Long
    For i = 3 To LastUsedRow
        If Worksheets("Home").Cells(i, 4).Value = "Active" Then
            Dim PasteColumn As Long

            Select Case Worksheets("Home").Cells(i, 3).Value
                Case "Denmark":        PasteColumn = 2 
                Case "Netherlands":    PasteColumn = 4
                Case "UK":             PasteColumn = 6
                Case Else:             PasteColumn = 0 'we need this to cancel copy
            End Select

            If PasteColumn > 0 Then
                Dim PasteLastRow As Long
                PasteLastRow = Worksheets("Practices").Cells(Rows.Count, PasteColumn).End(xlUp).Row

                Worksheets("Home").Cells(i, 2).Copy
                Worksheets("Practices").Cells(PasteLastRow + 1, PasteColumn).Paste
            End If
        End If
    Next i
End Sub
Pᴇʜ
  • 56,719
  • 10
  • 49
  • 73
0

I have had a go at what i think you mean. But there are many errors and inconsistencies throughout as noted in the comments.

Sub SortData()
    Dim a As Long, c As Range, sh  As Worksheet, ws As Worksheet, b As Long

    Set sh = ThisWorkbook.Sheets("Home")
    Set ws = ThisWorkbook.Sheets("Practices")
    a = sh.Cells(Rows.Count, 3).End(xlUp).Row

    For i = 3 To a
        If sh.Cells(i, 4).Value = "Active" Then
            Set c = sh.Range(Cells(i, "A"), Cells(i, "D"))
        End If

        If c.Columns(3) = "Denmark" Then
            b = ws.Cells(Rows.Count, 5).End(xlUp).Row
            c.Copy
            ws.Cells(i, 2).PasteSpecial
        ElseIf c.Cells(i, 3) = "Netherlands" Then
            b = ws.Cells(Rows.Count, 5).End(xlUp).Row
            c.Copy
            ws.Cells(i, 2).PasteSpecial
        ElseIf C.Cells(i, 3) = "UK" Then
            b = ws.Cells(Rows.Count, 5).End(xlUp).Row
            c.Copy
            ws.Cells(b + 1, 6).PasteSpecial
        End If
    Next
End Sub
Pᴇʜ
  • 56,719
  • 10
  • 49
  • 73
Barry
  • 53
  • 1
  • 1
  • 6
  • Thanks for having a go. I have a working version now. I just would like to know how to improve it so the code is more concise. I will edit the post now with the working code. – Infernez May 14 '18 at 10:30
  • @Barry I recommend always to format your code nicely (just did that). Otherwise it is pretty much unreadable for human beings and leads into many errors. Also it is very hard to maintain. • Also your code will fail if `sh.Cells(i, 4).Value = "Active"` is not "Active" because then `c` is nothing and there is nothing to copy! Also `b` is never used in the first 2 cases (in your `if` statement). So you should test and fix that. – Pᴇʜ May 14 '18 at 14:29
  • I am fully aware of the limitations thanks, just hoping to point the OP in the right direction as mentioned. – Barry May 14 '18 at 14:42