1

I had been using this code for a little while in a workbook, left and come back to revisit and found the code is no longer functioning as it once was. I cannot see any obvious mistakes and wondered if anyone could spot what perhaps would be stopping it running?

Page names and locations remain the same.

The purpose was to take results in Sheet 4 (CAL) and copy each row into a new empty line in RRR. No errors are displaying. Just nothing happens at all.

Sub ca_act()
    Dim nextrow As Long
    nextrow = Sheet4.Cells(4, "A").End(xlUp).Row + 1

    Dim src As Worksheet
    Set src = Sheets("CAL")

    Dim trgt As Worksheet
    Set trgt = Sheets("RRR")

    Dim i As Long
      For i = 1 To src.Range("y" & Rows.Count).End(xlUp).Row
        If src.Range("y" & i) = 1 Then
            ' calling the copy paste procedure
            CopyPaste src, i, trgt
        End If
    Next i
Application.ScreenUpdating = True
End Sub

' this sub copies and pastes the entire row into a different sheet
' below the last used row
Private Sub CopyPaste(ByRef src As Worksheet, ByVal i As Long, ByRef trgt As Worksheet)
    src.Activate
    src.Rows(i & ":" & i).Copy
    trgt.Activate
    Dim nxtRow As Long
    nxtRow = trgt.Range("y" & Rows.Count).End(xlUp).Row + 1
    trgt.Rows(nxtRow & ":" & nxtRow).PasteSpecial _
        Paste:=xlPasteAll, Operation:=xlNone, SkipBlanks:=False, Transpose:=False
End Sub
10
  • 3
    Can you be more specific than "is no longer functioning"? Error message, for example? Commented Dec 13, 2018 at 15:24
  • What is it doing incorrectly? What should it be doing? Commented Dec 13, 2018 at 15:24
  • 2
    Just added - read back and realized myself had missed this. Commented Dec 13, 2018 at 15:25
  • Step through the code with F8. When you get to the CopyPaste sub, is it functioning as you expect? Commented Dec 13, 2018 at 15:25
  • No nothing happens, page remains blank Commented Dec 13, 2018 at 15:32

1 Answer 1

1

Wrong Sheet or Column

Some Guess Work

The following line means that you will check values in column "A"

Dim nextrow As Long
nextrow = Sheet4.Cells(4, "A").End(xlUp).Row + 1

which was probably your first idea. BTW you should comment it out because it's useless.

Later you write

For i = 1 To src.Range("Y" & Rows.Count).End(xlUp).Row

which means you're checking column 'Y'. Are you sure about that?

I would consider the following:

  • You're checking for values in the wrong column.
  • Your sheets CAL and RRR might be wrong, maybe you have moved the name CAL e.g. to Sheet2 where there is no data.
  • In sheet 'RRR', you might have some unwanted data below in column 'Y' i.e. if you have accidentally put some data in a cell when it goes up it will stop at that cell and go one row down and write from there and you're not seeing it.
  • This is happening in different workbooks.

What's this all about

Application.ScreenUpdating = True

when

Application.ScreenUpdating = False

is nowhere to be found.

Here is the simplification of your second sub:

Private Sub CopyPaste(src As Worksheet, i As Long, trgt As Worksheet)
    src.Rows(i).Copy (trgt.Rows(trgt.Range("Y" & Rows.Count).End(xlUp).Row + 1))
End Sub

Simplification

Constants at the beginning of the code are lifesavers as you will probably see soon.

It is customary to release object variables when they're not needed anymore or at least at the end of the code. The following codes don't use any object variables which is achieved using the Parent property.

'*******************************************************************************
' Checks a column for a specified value, and each time it is found copies
' the entire current row to another worksheet below its last used row, using
' the CopyPaste_Simple Sub.
'*******************************************************************************
Sub ca_act_Simple()

    Application.ScreenUpdating = False

    Const strSource As Variant = "CAL"      ' Source Worksheet Name/Index
    Const strTarget As Variant = "RRR"      ' Target Worksheet Name/Index
    Const vntSourceCol As Variant = "Y"     ' Source Column Letter/Number
    Const lngSourceRow As Long = 1          ' Source First Row
    Const vntSearch as Variant = 1          ' Search Value

    Dim intRow As Long                      ' Row Counter

    With ThisWorkbook.Worksheets(strSource)
        For intRow = lngSourceRow To _
                .Cells(.Rows.Count, vntSourceCol).End(xlUp).Row
            If .Cells(intRow, vntSourceCol) = vntSearch Then
                ' calling the copy paste procedure
                CopyPaste_Simple .Parent.Worksheets(strSource), intRow, _
                    .Parent.Worksheets(strTarget)
            End If
        Next
    End With

    Application.ScreenUpdating = True

End Sub
'*******************************************************************************

'*******************************************************************************
' Copies the entire row to another worksheet below its last used row calculated
' from a specified column.
'*******************************************************************************
Sub CopyPaste_Simple(Source As Worksheet, SourceRowNumber As Long, _
        Target As Worksheet)

    ' It is assumed that the Target Worksheet has headers i.e. its first row
    ' will never be populated.

    Const vntTargetCol As Variant = "Y"     ' Target Column Letter/Number

    With Target
        Source.Rows(SourceRowNumber).Copy (.Rows(.Cells(.Rows.Count, _
            vntTargetCol).End(xlUp).Row + 1))
    End With

End Sub
'*******************************************************************************

Improvement

To improve we will get rid of the second sub:

'*******************************************************************************
' Checks a column for a specified value, and each time it is found copies
' the entire current row to another worksheet below its last used row
' calculated from a specified column.
'*******************************************************************************
Sub ca_act_Improve()

    Application.ScreenUpdating = False

    Const strSource As Variant = "CAL"      ' Source Worksheet Name/Index
    Const strTarget As Variant = "RRR"      ' Target Worksheet Name/Index
    Const vntSourceCol As Variant = "Y"     ' Source Column Letter/Number
    Const vntTargetCol As Variant = "Y"     ' Target Column Letter/Number
    Const lngSourceRow As Long = 1          ' Source First Row
    Const vntSearch as Variant = 1          ' Search Value         

    Dim intRow As Long                      ' Row Counter

    With ThisWorkbook.Worksheets(strSource)
        For intRow = lngSourceRow To _
                .Cells(.Rows.Count, vntSourceCol).End(xlUp).Row
            If .Cells(intRow, vntSourceCol) = vntSearch Then
                With .Parent.Worksheets(strTarget)
                    .Parent.Worksheets(strSource).Rows(intRow).Copy _
                    (.Rows(.Cells(.Rows.Count, vntTargetCol).End(xlUp).Row + 1))
                End With
            End If
        Next
    End With

    Application.ScreenUpdating = True

End Sub
'*******************************************************************************

In this improved version it is best visible that you are using column 'Y' in both worksheets, which might be the cause of your trouble.

The Second Sub

I think it's better to add the fourth argument:

'*******************************************************************************
' Copies an entire row to another worksheet below its last used row.
'*******************************************************************************
Sub CopyPaste_Improve(Source As Worksheet, SourceRowNumber As Long, _
        Target As Worksheet, TargetColumnLetterNumber As Variant)

    ' It is assumed that the Target Worksheet has headers i.e. its first row
    ' will never be populated.

    With Target
        Source.Rows(SourceRowNumber).Copy (.Rows(.Cells(.Rows.Count, _
            TargetColumnLetterNumber).End(xlUp).Row + 1))
    End With

End Sub
'*******************************************************************************
Sign up to request clarification or add additional context in comments.

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.