1

I have this sub in Excel 2010 which is supposed to filter through all the cells in a sheet until it finds a match to Proj No, then paste a field from this row into another field.

When I try to run the sub, it gives me an error 1004: Select Method of Worksheet Class Failed. I've marked the line where this occurs. Any assistance would be greatly appreciated.

Option Explicit
Private Sub btnNext_Click()

Dim ProjNo As String
Dim Col As String
Dim Row As String
Dim cell As Range

Unload Dialog
formWait.Show

Sheets("Sheet7").Activate

ProjNo = Worksheets("Sheet1").Range("D6").Value

Col = Cells(Rows.Count, "A").End(xlUp).Row

For Each cell In Range("A2:A" & Col)  
    If cell.Value = ProjNo Then
    Row = Row & cell.Row
    End If

Next cell
Workbooks("Form.xlsm").Sheets("Sheet7").Range("Row, 6").Copy Destination:=Sheets("Sheet1").Range("19, 5") ‘Error

Unload formWait

End Sub
AxxieD
  • 614
  • 1
  • 7
  • 28
  • It looks like you're specifying the `.Range` incorrectly. Try `.Range(.Sheets("Sheet7").Cells(Row, 6))`, no double-quotes -- does that work? – Dan Wagner Jul 21 '14 at 19:18
  • Also, this is a great opportunity to refactor your code to steer clear of using `.Active` and `.Select`, which are common sources of run-time errors. Here's SO's authoritative write-up on how to avoid both: http://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros – Dan Wagner Jul 21 '14 at 19:22
  • It first gave me a compile error at .Sheets so I removed the period but I still get the same error. – AxxieD Jul 21 '14 at 19:22

4 Answers4

1

There are a few confusing items in your code above, so I wanted to place them long-form here. Let's get started:

Dim Col As String
Dim Row As String

It looks like your design expects these to be of type Long rather than type String. Even if these variables were meant to be strings, I would recommend adjusting their names -- when your fellow developer attempts to review your design, he or she is likely to see names like "Col" or "Row" and think "these are numbers". Easy fix:

Dim Col As Long, Row As Long

The next issue comes up here:

Col = Cells(Rows.Count, "A").End(xlUp).Row

The structure above is a common method for identifying the last ROW, not column. (It also appears that you have switched the "A" and number, which is another easy fix). While it is perfectly acceptable syntactically to name the variable for last row "Col", human users are likely to find this confusing. Identifying the last row (and the last col, which you use in the For Each loop), as explained in fantastic detail here, would be better handled like this:

Dim SheetSeven As Worksheet, SheetOne As Worksheet
Dim LastRow As Long, LastCol As Long

Set SheetSeven = ThisWorkbook.Worksheets("Sheet7")
Set SheetOne = ThisWorkbook.Worksheets("Sheet1")

With SheetSeven
    LastRow = .Range("A" & .Rows.Count).End(xlUp).Row
    LastCol = .Range("A" & .Columns.Count).End(xlToLeft).Column               
End With

This should make your For Each loop look like this:

With SheetSeven
    For Each cell in .Range("A2:A" & LastCol)
    '... do you comparison and row incrementing here
    Next cell
End With

Once you've identified your sheet as a variable, the Range.Copy action should be much easier as well:

With SheetSeven
    .Range(.Cells(Row, 6)).Copy _
    Destination:=SheetOne.Range(SheetOne.Cells(19, 5))
End With
Community
  • 1
  • 1
Dan Wagner
  • 2,693
  • 2
  • 13
  • 18
1

I don't know what GWP is, but I think you want to use ProjNo there. The Range property doesn't accept an argument like that. Unless you have a named range of "Row,6" which you don't because it's not a legal name, you have to supply Range with a valid range reference, like A6 or D2:D12, for example.

Also, you can't concatenate rows and use them in a Range reference to get a larger range. You would have to copy each row inside the loop, union the ranges as you go, or better yet, filter on the value that you want and copy the visible rows.

Try this:

Private Sub btnNext_Click()

    With ThisWorkbook.Worksheets("Sheet7")
        'filter for the project id
        .Range("A1", .Cells(.Rows.Count, 1).End(xlUp)).Resize(, 6).AutoFilter 1, "=" & .Range("D6").Value

        'copy the visible rows
        .Range("F2", .Cells(.Rows.Count, 6).End(xlUp)).SpecialCells(xlCellTypeVisible).Copy _
            ThisWorkbook.Worksheets("Sheet1").Cells(19, 5)

        'get rid of the filter
        .AutoFilterMode = False

    End With

End Sub
Dick Kusleika
  • 32,673
  • 4
  • 52
  • 73
  • I'm just wondering, where does F2 come from? It's putting it in the right place but it isn't the value or the row I want. – AxxieD Jul 21 '14 at 20:07
  • You were copying `Range("Row, 6")` and F is the sixth column, so I assume you wanted that. You'll need to change the "6" in the AutoFilter line to make sure you get all the data. Then change F2 to whatever column you want. Then change the "6" in the Copy line to match the column that you change the F2 to. Or you could tell me what data you want and I will edit the code. – Dick Kusleika Jul 21 '14 at 22:18
1

Also one other thing you may wish to check is the status of Application.ScreenUpdating. With the release of Office 2013 and later, a SDI (Single Document Interface) was introduced. If Application.ScreenUpdating is False and the workbook is not active, the implied call to Workbook.Activate will fail. Check the status of ScreenUpdating and set it to True if needed. You can set it back to False after the first Activate call for that workbook is made. See this article: https://support.microsoft.com/en-us/help/3083825/excel-workbook-is-not-activated-when-you-run-a-macro-that-calls-the-wo

1

In my case the error came as the sheet was hidden. so I check if I am not working with the hidden sheet. Or you need to unhide the sheet before you try to select or activate sheet.

For Each sh In ThisWorkbook.Sheets
    If Left(sh.Name, 8) <> "Template" Then
        sh.Select
        sh.Range("A1").Select
    End If
Next
SUNIL KUMAR
  • 117
  • 5