Improving Code (97)

Topic

New Reply

As I’m just starting out with coding excel, I’m starting to notice that I use a lot of activecell.offset commands to navigate around my spreadsheets. Is this normal? Or are there more efficient ways to do this?

Here is an example of some code I have done, it works so I’m happy but I’m interested to learn if there are other methods being used by more advanced programmers.

Sub Totals3()

Dim dSub5 As Double
Dim dSub6 As Double

Range(“AF1”).Select
Do Until ActiveCell.Offset(0, -30).Value = “TOTAL – COST OF GOODS SOLD”
ActiveCell.Offset(1, 0).Select
If ActiveCell.Value = “PST” Then
dSub5 = ActiveCell.Offset(0, -7) + dSub5
dSub6 = ActiveCell.Offset(0, -4) + dSub6
Else
End If
Loop

ActiveCell.Offset(0, -7).Value = dSub5
ActiveCell.Offset(0, -4).Value = dSub6
ActiveCell.Offset(0, -3).Value = dSub5 – dSub6
ActiveCell.Value = “PSGT”

ActiveCell.Offset(0, -7).Range(“A1:H1”).Select
With Selection.Interior
.ColorIndex = 6
End With
Selection.Font.Bold = True
With Selection.Borders(xlEdgeTop)
.LineStyle = xlContinuous
End With

End Sub

Kind regards
Hayden

Reply | Quote

Replies

I see this code is just summing up two columns untill it reaches a “bottom” row. Why not use the =SUM worksheet function in stead of a macro?

Otherwise, your code could also look like this (untested!):

Sub Totals3()
Dim lCount as long
Dim dSub5 As Double
Dim dSub6 As Double
Dim oCell as range
Set oCell=Range(“AF1”)
Do Until oCell.Offset(lcount, -30).Value = “TOTAL – COST OF GOODS SOLD”
lcount=lcount+1
If ocell.Offset(lCount).Value = “PST” Then
dSub5 = oCell.Offset(lCount, -7) + dSub5
dSub6 = oCell.Offset(lCount, -4) + dSub6
End If
Loop

oCell.Offset(lCount, -7).Value = dSub5
oCell.Offset(lCount, -4).Value = dSub6
oCell.Offset(lCount, -3).Value = dSub5 – dSub6
oCell.Offset(lCount).Value = “PSGT”

Reply | Quote

Of course I meant to write “the SUMIF function”….

Reply | Quote

Of course I meant to write “the SUMIF function”….

Reply | Quote

I see this code is just summing up two columns untill it reaches a “bottom” row. Why not use the =SUM worksheet function in stead of a macro?

Otherwise, your code could also look like this (untested!):

Sub Totals3()
Dim lCount as long
Dim dSub5 As Double
Dim dSub6 As Double
Dim oCell as range
Set oCell=Range(“AF1”)
Do Until oCell.Offset(lcount, -30).Value = “TOTAL – COST OF GOODS SOLD”
lcount=lcount+1
If ocell.Offset(lCount).Value = “PST” Then
dSub5 = oCell.Offset(lCount, -7) + dSub5
dSub6 = oCell.Offset(lCount, -4) + dSub6
End If
Loop

oCell.Offset(lCount, -7).Value = dSub5
oCell.Offset(lCount, -4).Value = dSub6
oCell.Offset(lCount, -3).Value = dSub5 – dSub6
oCell.Offset(lCount).Value = “PSGT”

Reply | Quote

Hi Hayden

Just like you, I have just really got to grips with Excel VBA and yes, I use offset quite a lot too but recently I have found that because of other projects I am working on I am having to use other functions too. I have only learnt by crawling through old posts and seeing other peoples examples. My view, and probably wrong, is that for small applications I can’t see a problem with it.

Jerry

Reply | Quote

Hi Hayden

Just like you, I have just really got to grips with Excel VBA and yes, I use offset quite a lot too but recently I have found that because of other projects I am working on I am having to use other functions too. I have only learnt by crawling through old posts and seeing other peoples examples. My view, and probably wrong, is that for small applications I can’t see a problem with it.

Jerry

Reply | Quote

I’m a self-taught VBA guy, but one thing I’ve learned is to never use a cell reference in VBA if at all possible. One of your first lines is range(“af1”).select. If someone inserts a row or column, the cell refs in your VBA will no longer be correct. If possible, it’s better to use range names in your VBA. In your example, it looks like you’re receiving data from someone else and you want to automate adding some formulas to it, so it may not make sense to create a range name.

It looks to me like you’re looking down Column B until you get to CGS. For each row, if column AF has “PST”, then update running total dsub5 with the value in column Y and dsub6 with the value in column AB. I’m not sure this is best solved using a macro. Have you tried using the SUMIF function? Perhaps =SUMIF($AF1:$AF50,”PST”,Y1:Y50) at the bottom of column Y and then copy it over to the bottom of column AB.

You can delete the ELSE line if there’s nothing to do when Activecell.value “PST”

You can simplify the formatting part of your macro a little bit:

With Selection
Interior.ColorIndex = 6
Font.Bold = True
.Borders(xlEdgeTop).LineStyle = xlContinuous
End With

I’m not sure how many rows of data you have to process, but if it’s thousands, you can speed up execution by putting Application.Screenupdating=False at the beginning of your macro and Application.Screenupdating=True at the end. This turns off redrawing the display during execution.

Reply | Quote

Thanks for your input and thoughts everyone, much obliged!

Reply | Quote

Could you tell us what you wanted your macro to do? Then, we could have a contest to see who can come up with the “best” code!

Putting on my VBA teacher’s hat:
1) Always have Option Explicit as the first line of code in the module. This forces you to declare every variable. It’s too easy to misspell a variable name and it will cost you lots of time to find the mistake.

2) Never use the Select method.

3) Never directly reference a cell unless it is a named range. In your worksheet, you should give “AF1” a name, eg Cost. Then in code you can use [Cost]. or Range(“Cost”). to reference the cell.

4) Try to think of a simpler way to code than using Offset. I think that Offset makes the code hard to understand. I very rarely use it.

5) Always initialize an accumulator. VBA will probably change someday and not init everything to 0.

6) Read sample code in this forum and decide whether it looks good to you.

Have fun and believe it or not Excel is the easiest Office product to write code for! –Sam

Reply | Quote

Hello Sam,

Yes certainly. The macro forms just one part of a huge piece of code I have been writing (Copy attached for your reference) which re-configures sales data from our London sales team. We trade in multi-currencies (E.g.: buy in USDollars and sell in Euro

Reply | Quote

[indent]

---

When we receive the sales report the idea is to work through the rows and pull out, convert and sub-total the relevant data and sections

---

[/indent]And exactly there lies the key to helping you with this problem:

What is to be pulled out, what is relevant to sub-total?
What is the layout of this report that needs to be worked over (column order always the same or not, differening amount of rows or not,……)

Reply | Quote

[indent]

---

When we receive the sales report the idea is to work through the rows and pull out, convert and sub-total the relevant data and sections

---

[/indent]And exactly there lies the key to helping you with this problem:

What is to be pulled out, what is relevant to sub-total?
What is the layout of this report that needs to be worked over (column order always the same or not, differening amount of rows or not,……)

Reply | Quote

Hello Sam,

Yes certainly. The macro forms just one part of a huge piece of code I have been writing (Copy attached for your reference) which re-configures sales data from our London sales team. We trade in multi-currencies (E.g.: buy in USDollars and sell in Euro

Reply | Quote

Some good guidelines. I disagree with:

[indent]

---

2) Never use the Select method

---

[/indent]

I think this is a bit strict. (Maybe it is me, but) I have found to work with some objects you MUST select them to get to any of the properties. Some objects will not let you work directly with them, you must SELECT them and then work with the SELECTION.

Also (in XL97) you must remove the focus from controls used to call code in VB in certain circumstances or you will get a runtime error. SInce many of the controls do not have a TakeFocusonClick Property to change to false, the easiest way is to use the command activecell.select and the control will NOT have the focus.

I think the select method should be AVOIDED (and NEVER used to move around a spreadsheet), but has its uses.

Steve

Reply | Quote

Some good guidelines. I disagree with:

[indent]

---

2) Never use the Select method

---

[/indent]

I think this is a bit strict. (Maybe it is me, but) I have found to work with some objects you MUST select them to get to any of the properties. Some objects will not let you work directly with them, you must SELECT them and then work with the SELECTION.

Also (in XL97) you must remove the focus from controls used to call code in VB in certain circumstances or you will get a runtime error. SInce many of the controls do not have a TakeFocusonClick Property to change to false, the easiest way is to use the command activecell.select and the control will NOT have the focus.

I think the select method should be AVOIDED (and NEVER used to move around a spreadsheet), but has its uses.

Steve

Reply | Quote

Could you tell us what you wanted your macro to do? Then, we could have a contest to see who can come up with the “best” code!

Putting on my VBA teacher’s hat:
1) Always have Option Explicit as the first line of code in the module. This forces you to declare every variable. It’s too easy to misspell a variable name and it will cost you lots of time to find the mistake.

2) Never use the Select method.

3) Never directly reference a cell unless it is a named range. In your worksheet, you should give “AF1” a name, eg Cost. Then in code you can use [Cost]. or Range(“Cost”). to reference the cell.

4) Try to think of a simpler way to code than using Offset. I think that Offset makes the code hard to understand. I very rarely use it.

5) Always initialize an accumulator. VBA will probably change someday and not init everything to 0.

6) Read sample code in this forum and decide whether it looks good to you.

Have fun and believe it or not Excel is the easiest Office product to write code for! –Sam

Reply | Quote

Thanks for your input and thoughts everyone, much obliged!

Reply | Quote

I’m a self-taught VBA guy, but one thing I’ve learned is to never use a cell reference in VBA if at all possible. One of your first lines is range(“af1”).select. If someone inserts a row or column, the cell refs in your VBA will no longer be correct. If possible, it’s better to use range names in your VBA. In your example, it looks like you’re receiving data from someone else and you want to automate adding some formulas to it, so it may not make sense to create a range name.

It looks to me like you’re looking down Column B until you get to CGS. For each row, if column AF has “PST”, then update running total dsub5 with the value in column Y and dsub6 with the value in column AB. I’m not sure this is best solved using a macro. Have you tried using the SUMIF function? Perhaps =SUMIF($AF1:$AF50,”PST”,Y1:Y50) at the bottom of column Y and then copy it over to the bottom of column AB.

You can delete the ELSE line if there’s nothing to do when Activecell.value “PST”

You can simplify the formatting part of your macro a little bit:

With Selection
Interior.ColorIndex = 6
Font.Bold = True
.Borders(xlEdgeTop).LineStyle = xlContinuous
End With

I’m not sure how many rows of data you have to process, but if it’s thousands, you can speed up execution by putting Application.Screenupdating=False at the beginning of your macro and Application.Screenupdating=True at the end. This turns off redrawing the display during execution.

Reply | Quote