Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This code gets the cell's colour regardless of whether it's set from conditional formatting or not. It currently works on 2010 and unsure about older versions of excel.

My question is is there a way to make this code more efficient?

  • Such as being able to highlight an area for the code to search rather than setting For stantments

  • Getting it to search for a specific colour you set rather than red or whatever has been set

  • Anything else anyone can think of

Sub myCFtest()

For q = 1 To 26

sCol = Split(ActiveCell.Address, "$")(1)
sColNum = sCol & 1
Range(sColNum).Select

For i = 1 To 100

sColNum = ActiveCell.Address

    If Range(sColNum).DisplayFormat.Interior.Color = 255 Then
        Y = ActiveCell.Address
        MsgBox ("Red Cell Found At " & Y)
    End If

ActiveCell.Offset(1, 0).Select

Next i

ActiveCell.Offset(0, 1).Select

Next q

MsgBox ("No Red Cell Found")

End Sub
share|improve this question
2  
DisplayFormat was introduced in 2010. – Raystafarian Jan 26 at 12:39
2  
Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Vogel612 Jan 26 at 14:52
    
@Raystfarian @Zak, Thank you both for your advice for my code, seems I cant show you the updated code but I have added Dims into the code so you can understand what each varible is doing and I have changed the naming convention so it is easy to understand what each is going to do, I have also updated it so it is using more varibles rather than ActiveCell and .Select though it is still using some(i'm not that good yet). Still Thank you both greatly for your input on my code and I hope I will be able to be able to code as well as you all – Mr.Burns Jan 26 at 15:10
1  
If you made changes and you still have things you want to improve, you can post another question about it. However, if it's as good as you need it to be, no need to do that. Just try to remember the improvements you made to this and incorporate them into future code, which you can bring back here to get help with as well! – Raystafarian Jan 26 at 15:16
    
@Raystfarian Currently the code isnt where I quite want it to be however I know what I need to do to improve it thanks yours and Zaks guidance and as easy as it would be to post it again it means I dont get the experience of learning this for myself(which is 90% the reason im coding) which is an incredible feeling when you worked out something for your self. (I feel I need to add something about it being fine to ask for help but I cant word it right(but you get the gist of it(hope no one takes anything the wrong way))) – Mr.Burns Jan 26 at 15:24
up vote 6 down vote accepted

I can't test this code to see if it works because I have 2007 on this computer. But, I do have some things to add.

First things first, I have no idea what the variables are for - there is no description. You also don't have Option Explicit on and none of the variables are dimed. That's the first thing to address.

What is q? It iterates 1 to 26, but I don't see it being used anywhere. You're just doing the entire thing 26 times? Why? Oh, because you're using .Select - I'll get to that.

The same thing goes to i.

You use sCol = Split(ActiveCell.address,"$")(1) to get the column letter? Why not just get the column with ActivecCell.Columns?

Speaking of ActiveCell - why are you using it? Why not get a variable like Dim RangeToTest and set it to Sheets(1).Range("A1:A26") or whatever? At least Set RangeToTest = Selection - but using selection and activecell is generally bad form.

Instead this entire thing could be (using whatever numbers you need to use)-

  Dim RowNumber As Long
  Dim ColumnNumber As Long

  For RowNumber = 1 To 100
    For ColumnNumber = 1 To 26
        'do stuff to Cells(RowNumber, ColumnNumber)
    Next ColumnNumber
  Next RowNumber

If you're not sure you can do something like this to get what you need -

  Dim RowNumber As Long
  Dim ColumnNumber As Long
  Dim LastRow As Long
  LastRow = Cells(Rows.Count, "A").End(xlUp).Row
  Dim LastColumn As Long
  LastColumn = Cells(1, Columns.Count).End(xlToLeft).Column

  For RowNumber = 1 To LastRow
    For ColumnNumber = 1 To LastColumn
        'do stuff to Cells(RowNumber, ColumnNumber)
    Next ColumnNumber
  Next RowNumber

I can't tell what you're doing here -

sCol = Split(ActiveCell.Address, "$")(1)
sColNum = sCol & 1
Range(sColNum).Select

For i = 1 To 100

sColNum = ActiveCell.Address

From what I can tell you get the column, set sColNum (is that a string? A range?) to the first cell in the column, then you select the Range of the SColNum (string?). And then you set the sColNum that was selected to its own .address? What are you accomplishing here? Seems like it has something to do with your i loop - which can be eliminated and you can just use the example I gave, or something similar.

Now you check for .Interior.Color = 255 and if it's found, you msgbox where it was found with Y as activecell.address when sColNum is already the activecell.address. You don't need Y to say the least.

Instead you could do something like this -

Dim FindInteriorColor As Long

  FindInteriorColor = 255

  If Cells(RowNumber, ColumnNumber).DisplayFormat.Interior.Color = FindInteriorColor Then
    MsgBox ("Red Cell Found at " & Cells(RowNumber, ColumnNumber))
  End If

Now all you need to do is change the variable FindInteriorColor to whatever the value is of the color you want to find. You'd still need to change your msgbox to say the color, but that could be avoided with an input box or variable or something.

Again, I can't test the actual function of the code, but these are improvements you could make at least, if you want to run through cell by cell looking for something and saying each time it's found. If you want to only find the first one, just put an Exit Sub in the if loop.


In response to your comment - a simple way to get the letter of a column number (in case you need it in the future) is something like -

Sub test()
Dim RowBegin As Long
RowBegin = InStr(2, Cells(1, 200).Address, "$")
Dim ColumnLetter As String
ColumnLetter = Mid(Cells(1, 200).Address, 2, RowBegin - 2)
MsgBox ColumnLetter
End Sub
share|improve this answer
    
Thanks for your answer @Raystafarian, i use sCol = Split(ActiveCell.address,"$")(1) as it gives the letter of a column instead of the number and other code I have found to convert it it from number to letter doesnt fit well, least not the ones I found. I am currently updating the code with yours and Zak's suggestion and will put the code up when it is done – Mr.Burns Jan 26 at 14:21
    
Ah, I added something at the end of the answer in case you ever do need to go from number to letter of a column. – Raystafarian Jan 26 at 14:34
    
Thanks for your code I will look at it a bit more, it gives the coloumn letter which is good but seems i will need to learn a bit more about the Cells function for it to work the way I need it to – Mr.Burns Jan 26 at 15:06

Your code is incredibly difficult to follow

Naming, one of the most important (and hardest) parts of any software development. Software Dev. is about 80% reading code and only about 20% writing it.

More specificaly, that 80% is not so much reading code as trying to understand it. Short, concise code is only better if it is easier to understand. Shorter names are only better if they are easier to understand.

variableThatHoldsThisThing, despite being very long is a far better name than vThngHldr, even if the latter "looks" cleaner.

Moreover, variables should sound like what they are.

sCol is not a good name. I have no idea what it is or what it should be. columnLetter on the other hand is only slightly longer and instantly tells me what it is.

sColNum is even worse because it's not a column number. It's the string address of the first cell in the column. cellAddress is clear, concise and, most importantly, is an accurate description of what it holds.

Which is clearer?

sCol = Split(ActiveCell.Address, "$")(1)
sColNum = sCol & 1
Range(sColNum).Select

or

columnLetter = Split(ActiveCell.Address, "$")(1)
cellAddress = columnLetter & 1
Range(cellAddress).Select

But, then we get to the worst variable of them all: Y

First off, it is a universal programming convention that single-letter variables are counters. Usually used for loops, indexes, iterations etc. Using them for anything else is a huge problem.

Using them for something else that is not a number is even worse. If you have a variable that represents a cell Address, then call it cellAddress.

But wait, isn't sColNum already the cell address? So Y is redundant anyway (which is much easier to spot when you give your variables descriptive names).

sColNum = ActiveCell.Address

    If Range(sColNum).DisplayFormat.Interior.Color = 255 Then
        Y = ActiveCell.Address
        MsgBox ("Red Cell Found At " & Y)
    End If

is exactly the same as

sColNum = ActiveCell.Address

    If Range(sColNum).DisplayFormat.Interior.Color = 255 Then
        MsgBox ("Red Cell Found At " & sColNum)
    End If

On the other hand, q and i are perfectly fine.

share|improve this answer
    
Thanks for your answer @Zak I am currently updating my code and will put it up when it is done and would like to see what you think of the updated version – Mr.Burns Jan 26 at 14:22
    
@Mr.Burns Awesome, please see this before you do. – Zak Jan 26 at 14:26
    
Yea I have that saved in my favorties as well but currently I cant quite get my head around it yet, though once I do I will use it, most likely all the time – Mr.Burns Jan 26 at 14:52
    
Very simply, when you improve your code, write a new question, put a link to this one at the top. And a link to the new one at the top of this one. Then treat the new question as its' own separate entity. – Zak Jan 26 at 15:18

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.