6
\$\begingroup\$

I'm currently writing code so that when a red-colored cell appears in a column, a message box will appear letting me know that column has an error. I have written the following method:

Sub MSG()

    Dim a
    Dim B

    With Application.FindFormat.Interior
        .color = vbRed
    End With

    '/ Sheet1 is example sheet name
    Set a = Sheet1.Columns(1).Find(What:="", SearchFormat:=True)
    Set B = Sheet1.Columns(2).Find(What:="", SearchFormat:=True)

    If Not a Is Nothing Then
          MsgBox ("There is an issue with column A, please review.")
    End If
    If Not B Is Nothing Then
          MsgBox ("There is an issue with column B, please review.")
    End If

End Sub

But as you can see, expanding this to support an arbitrary number of columns would be impossible, and going up to column ED would require 134+ lines of code to be written!

Is there a more efficient way to do this? Would it be possible to optimize this code?

\$\endgroup\$
4
  • 4
    \$\begingroup\$ You need a loop. Should be discussed in your VBA language reference of choice. \$\endgroup\$
    – Cody Gray
    Jan 25 '17 at 14:44
  • \$\begingroup\$ That's only written for column A and B, I need to to check every column up to ED so that would be around 134+ lines roughly. \$\endgroup\$
    – Hosey93
    Jan 25 '17 at 14:44
  • \$\begingroup\$ Ah I've no idea how to do loops yet, was looking for a quick fix if anyone knew how. Thanks for your help though. \$\endgroup\$
    – Hosey93
    Jan 25 '17 at 14:45
  • \$\begingroup\$ Thank you @CodyGray for editing my question and making it understandable. Again, I greatly appreciate your help. \$\endgroup\$
    – Hosey93
    Jan 25 '17 at 14:51
7
\$\begingroup\$

You found Code Review in 5 days. Congratulations! It took me over a year.


The most important single lesson I want you to take away is this:

Code must be designed to be read and understood as easily as possible.

It doesn't matter how good the code is, if nobody can understood what it's doing (and that includes you when you need to come back in a day/week/month/year and fix/change/improve it) then it is useless. It could be the best code in the world and it would be entirely useless to anybody who actually needs to use it.

Which brings us to our most important subject:


Naming

MSG(), a, B

These names are not good because I have no idea what they are or what they are doing.

In contrast, take a look at this and see how long it takes you to figure out what's going on.

Public Sub FindRedCells()
    '/ Searches a target sheet for cells colored vbRed and msgBox-es each column that contains one

    '/ Set Search format
    Application.FindFormat.Interior.Color = vbRed

    '/ Define the columns to search
    Const FIRST_SEARCH_COLUMN As Long = 1 '/ Column "A"
    Const LAST_SEARCH_COLUMN As Long = 134 '/ Column "ED"

    Dim targetSheet As Worksheet
    Set targetSheet = Sheet1

    Dim columnRange As Range
    Dim foundCell As Range
    Dim iColumn As Long
    For iColumn = FIRST_SEARCH_COLUMN To LAST_SEARCH_COLUMN 

        Set columnRange = targetSheet.Columns(iColumn)
        Set foundCell = columnRange.Find(What:="", SearchFormat:=True)

        If Not foundCell Is Nothing Then
            '/ Found a matching cell

            MsgBox "There is an issue with column (" & iColumn & "), please review."

        End If

    Next iColumn

End Sub

Names must be descriptive, unambiguous, and only then concise. When you're starting out, long names are your friend. Be obvious and explicit about what you're doing. If you can't look at a variable name and immediately know what it's supposed to be doing, then the name isn't descriptive enough.


Hopefully the above also demonstrates loops for you.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You need to complete the <For ... Next> structure \$\endgroup\$ Jan 25 '17 at 15:29
  • 4
    \$\begingroup\$ Hey, @Kaz the above solution that you have given is a great learning element and is exactly what I needed. I've only been learning how to script in VBA with essentially google and a few questions here and there but with answers like yours and Cody Gray, I'll be an expert in no time. I'm greatly appreciative of the help everyone in this community has provided. \$\endgroup\$
    – Hosey93
    Jan 25 '17 at 16:26
  • 1
    \$\begingroup\$ @Hosey93 Glad to hear it. Keep coming back to CR and you'll be a great developer in no time. Give this question 24 hours or so to potentially attract more answers and then you may be interested in an Iterative Review \$\endgroup\$
    – Kaz
    Jan 25 '17 at 16:31
7
\$\begingroup\$

The basic construct you're looking for is a loop. This is what you should reach for any time you find yourself writing the same code (performing the same operation) over and over again, but in a regular pattern (e.g., on columns A–ED).

There are several different types of loops supported by VBA (consult your favorite language reference for details), but you probably want a ranged loop here, since you want to loop through columns A through ED (or whatever maximum column). In VBA parlance, that would be a For loop; something like this:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For col = 1 To 134
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & col & "; please review."
        End If
    Next
End Sub

Notice that I have made the following additional improvements to your original code:

  • I have replaced MSG with a meaningful and descriptive name for the subroutine. This might not be the perfect name, but it is certainly a major improvement. It's also less likely to conflict with other identifiers (msg is pretty common, and VBA isn't case-sensitive).

  • The WithEndWith construct is only useful when you are setting multiple properties at a particular level of nesting. Your With block only had one statement inside of it, so I've removed it and replaced it with a single statement.

  • I believe variables should be declared as close as possible to the point of use. Ideally, they would be declared and initialized all at once, but if I remember correctly, VBA doesn't actually support this; you have to break it up into two separate lines. Still, keep 'em close! Also, keep them as narrowly scoped as possible—notice that here, I've put the variable declaration inside of the loop, instead of outside.

  • If a subroutine takes only one argument, you should not wrap its argument list in parentheses. Thus, I've removed the parentheses around the string passed in the call to the MsgBox subroutine.

But the code I've written above isn't quite what you want—we still need to fix a couple more things!

The first issue is that it will display a message box containing the numeric ID of the problematic column, rather than the alphabetic ID displayed in the Excel UI. For your user's sake, you probably want to change this. Since you're using a loop with the numeric column ID as the loop counter, you need an algorithm to systematically convert that numeric ID to the alphabetic ID. I would define a reusable function for this. The following function is adapted from one appearing in a Microsoft support article:

Function ColumnNumberToLetter(col As Integer) As String
    Dim alpha As Integer
    alpha = col \ 27

    Dim remainder As Integer
    remainder = col - (alpha * 26)

    If alpha > 0 Then
        ColumnNumberToLetter = Chr(alpha + 64)
    End If

    If remainder > 0 Then
        ColumnNumberToLetter = ColumnNumberToLetter & Chr(remainder + 64)
    End If
End Function

Then, just plug in a call to this function in the appropriate spot:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For col = 1 To 134
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

Also, now that we've got a loop, we no longer have to arbitrarily limit ourselves to going through column ED. It is simple to tweak this code to support any number of columns in a sheet—you just make the loop's upper bound the sheet's last column:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    Dim totalColumns As Integer
    totalColumns = Sheet1.Columns.Count

    For col = 1 To totalColumns
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

In fact, though, we can do even better. VBA supports a special type of For loop that can be used to automatically iterate through an entire collection of values (in this case, all of the columns in a sheet). This is the For Each loop, and it can be used like so:

Sub ShowMsgForErrorColumns()
    Application.FindFormat.Interior.Color = vbRed

    For Each col In Sheet1.Columns
        Dim match
        Set match = Sheet1.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

This is much clearer, less error-prone (it no longer makes assumptions about what the first column is), and more extensible. Almost perfect!

One last thing—instead of hard-coding which sheet you want this function to work on, consider passing in the sheet as an argument:

Sub ShowMsgForErrorColumns(sheet As Worksheet)
    Application.FindFormat.Interior.Color = vbRed

    For Each col In sheet.Columns
        Dim match
        Set match = sheet.Columns(col).Find(What:="", SearchFormat:=True)

        If Not match Is Nothing Then
            MsgBox "There is an issue with column " & ColumnNumberToLetter(col) & "; please review."
        End If
    Next
End Sub

That makes this function reusable. You don't want to go too far with reusability (YAGNI), but at the same time, you don't want to hard-code assumptions or arbitrarily box yourself in. Sometimes, it's just more natural to parameterize a function.

The only thing that bothers me about this code is that setting the Application.FindFormat.Interior.Color property has global side-effects that are visible outside of the function. This bothers me for the same reason that global variables make my skin crawl. One way to solve this would be to save the original value at the top of the function, and then restore it at the end. But that might not be worth it, if you are disciplined about always making sure to set this property to the desired value before any calls to the Find function.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You are a life saver, some top quality information to read through as well. I didn't realize that loops were so important when creating VBA functions. \$\endgroup\$
    – Hosey93
    Jan 25 '17 at 15:23
  • \$\begingroup\$ "Ideally, they would be declared and initialized all at once, but if I remember correctly, VBA doesn't actually support this" The following is perfectly valid: Dim match: Set match = sheet.Columns(col).Find(What:="", SearchFormat:=True), although not exactly "declared and initialized all at once", rather executing two statements in a single line. Other than that, awesome explanation. Very detailed and educative. \$\endgroup\$ Jan 25 '17 at 18:14
  • 2
    \$\begingroup\$ I actually thought about mentioning that when I was writing the answer, @victor, but decided that it is technically still two lines and so my phrasing was sufficiently correct. To be honest, that is the way I would personally write it, as a C++ programmer, but I don't think it's idiomatic VBA, and I didn't think it made sense to bring it up to someone who said they were a beginner. Thanks for the complement, though, and probably good to mention this in the comments for completeness! \$\endgroup\$
    – Cody Gray
    Jan 25 '17 at 18:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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