Wrote a VBA script that makes sense but still doesn't work

Question

Hi so I am trying to run the following script but all it does is put "N/A" in a column and just loop through all the other cells below without doing anything.

Sub NA()
    Range("E1").Select
    Do Until IsEmpty(ActiveCell)
        If Range("E1") = "password_update" Then If Range("G1") = Range("J1") Then Range("B1") = "N/A"
        ActiveCell.Offset(1, 0).Select
    Loop
End Sub

What I'm trying to do is check if the cell "E1" has the value "password_update" in it and if yes then also check if cell "G1" and cell "J1" has matching content, and if all these criteria matches then type "N/A" to cell "B1".

I also wanted to have this script loop through all subsequent rows (E1 then E2 then E3 and so on you get the idea). Now I am no VBA expert, but I wrote this script and no matter how many times I look it makes sense to me logically but it still doesn't work. Any ideas? Help would be appreciated.

score 1 · Accepted Answer · answered Oct 11 '17 at 07:47

1

Try this. Whenever possible, avoid using select statement in your code.

Sub NA()
lastrow = Cells(Rows.Count, "E").End(xlUp).Row
For X = 1 To lastrow
    If Range("E" & X).Value = "password_update" And Range("G" & X) = Range("J" & X) Then Range("B" & X) = "N/A"
Next X
End Sub

answered Oct 11 '17 at 07:47

Kresimir L.

2,301
2
10
22

Ha, thanks this is so much simpler than my code. I never used loops with Next and "X" as a variable before. Just curious by the way why is it advised not to use select statement? All beginner guides are all over it. Either way thanks for the help, I altered the code to match the report I am working on and it works wonders. – Rhyfelwr Oct 11 '17 at 07:57
You want to avoid 'visiting' the sheet where ever possible as this slows your code. You want to work in memory where possible. [Avoid select](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba) – QHarr Oct 11 '17 at 08:00
1

When used in large loops, select statement makes code much slower, and in general select is more error prone....please mark as answered. – Kresimir L. Oct 11 '17 at 08:00
Ah okay I see. I have another (working) macro that I uses .Select and it is indeed a bit slower to run through 1000s of rows (like 7-8 seconds). I'll optimize the code according to this then. Thank you guys, marked as answered. I guess I got accustomed to .Select because of starting out with recorded macros and then working with that, macro recording uses .Select all the time. – Rhyfelwr Oct 11 '17 at 08:14

score 0 · Answer 2 · answered Oct 11 '17 at 08:04

Similar to code above but if you want to be more explicit about what is happening and the sheets you are using (in case you run when in the wrong sheet for example consider putting something similar to below). This uses the same logic as you used

Sub UpdateColBwithNA() 'Place in a standard module
    Dim wb As Workbook
    Dim ws As Worksheet
    Dim LastRow As Long
    Dim currentRow As Long

    Set wb = ThisWorkbook
    Set ws = wb.Sheets("Sheet1") 'Change to name of sheet you are running code against

    With ws

        LastRow = .Cells(.Rows.Count, "A").End(xlUp).Row
        .Range("E1").Activate

        For currentRow = 1 To LastRow
            If .Cells(currentRow, "E") = "password_update" And _
               (.Cells(currentRow, "G") = .Cells(currentRow, "J")) Then
                    .Cells(currentRow, "B") = "N/A"
            End If
        Next currentRow
        End With
End Sub

The With statement means you don't have to go to the sheet.

Thank you, this one works too. I usually work on just generic "Sheet1", but I know that it is better to be explicit about which Sheet/Workbook I am working on. Should be less lazy I guess haha. Thanks for the suggestion! — Rhyfelwr, Oct 11 '17 at 08:16
Your current activeworkbook may not be the one your code, as written, is expecting, so the reason for being explicit was to ensure you targeted the right range, in the right sheet, in the right workbook. — QHarr, Oct 11 '17 at 08:18

Wrote a VBA script that makes sense but still doesn't work

2 Answers2