Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have an Excel Worksheet consisting of two columns, one of which is filled with strings and the other of which is empty. I would like to use VBA to assign the value of the cells in the empty column based on the value of the adjacent string in the other column.

Dim regexAdmin As Object 
Set regexAdmin = CreateObject("VBScript.RegExp") 
regexAdmin.IgnoreCase = True
regexAdmin.Pattern = "Admin" 

Dim i As Integer
For i = 1 To 10 'let's say there is 10 rows
    Dim j As Integer
    For j = 1 To 2
        If regexAdmin.test(Cells(i, j).Value) Then
            Cells(i, j + 1).Value = "Exploitation"
        End If
    Next j
Next i

The problem is that when using this loop for a big amount of data, it takes way too long to work and, most of the time, it simply crashes Excel.

Does anyone know a better way of doing this?

share|improve this question
    
Welcome to Code Review by the way. It's great to have you here. We don't get enough vba questions. –  RubberDuck Aug 7 at 16:28

3 Answers 3

Wow. Just reading through the first couple of lines got me wondering:

  • Why the late-binding?
  • Why use a regex at all?

@ckuhn203 already addressed the naming in his answer, but I find this:

Dim i As Integer
For i = 1 To 10 'let's say there is 10 rows

Turned into that:

Dim row As Integer
For row = 1 To 10 'let's say there is 10 rows

...Doesn't need the comment anymore.


I would like to use VBA to assign the value of the cells in the empty column based on the value of the adjacent string in the other column.

I think that's [mis|ab]using VBA: Excel itself is very good at dealing with assigning cell values based on other cells' values.


regexAdmin.Pattern = "Admin" 

I think that's [mis|ab]using regex: if your pattern is just a plain word, you're most probably trying to kill a mosquito with a bazooka. Wrong tool for the job here.

share|improve this answer

The short answer is:

Don't use , use a formula. In particular, a combination of IF and SEARCH.

=IF(SEARCH($A1,"Admin")>0,"Exploitation","")

But this is code review, so let's do that anyway.

  • Regex is slow. It seems that you're only using it for it's case insensitivity. Given that, you can directly compare cell values by using StrComp with the vbTextCompare option. (useful article on StrComp)

  • i and j are typically used for loop counters, but row and col make more sense in this case.

Here is what this could could look like:

Dim row As Integer
For row = 1 To 10 'let's say there is 10 rows
    Dim col As Integer
    For col = 1 To 2
        If StrComp("Admin",Cells(row, col).Value,vbTextCompare) Then
            Cells(row, col + 1).Value = "Exploitation"
        End If
    Next col
Next row
share|improve this answer

I would think that simple string comparison would be much faster than Regex.

Dim pattern as string
pattern = "Admin"
Dim i As Integer
For i = 1 To 10 'let's say there is 10 rows
    Dim j As Integer
    For j = 1 To 2
        If Cells(i, j) = pattern Then
            Cells(i, j + 1) = "Exploitation"
        End If
    Next j
Next i
share|improve this answer
    
This code doesn't address case insensitive comparison, but you're correct. The regex is overkill and string comparison is preferred in this case. (Welcome to Code Review by the way!) –  RubberDuck Aug 7 at 18:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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