Results 1 to 10 of 10
  1. #1
    2 Star Lounger
    Join Date
    Mar 2003
    Location
    London, Gtr London
    Posts
    131
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Improving Code (97)

    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

  2. #2
    Platinum Lounger
    Join Date
    Feb 2001
    Location
    Weert, Limburg, Netherlands
    Posts
    4,812
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    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"
    Jan Karel Pieterse
    Microsoft Excel MVP, WMVP
    www.jkp-ads.com
    Professional Office Developers Association

  3. #3
    Platinum Lounger
    Join Date
    Feb 2002
    Location
    A Magic Forest in Deepest, Darkest Kent
    Posts
    5,681
    Thanks
    0
    Thanked 1 Time in 1 Post

    Re: Improving Code (97)

    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
    Jerry

  4. #4
    Platinum Lounger
    Join Date
    Feb 2001
    Location
    Weert, Limburg, Netherlands
    Posts
    4,812
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    Of course I meant to write "the SUMIF function"....
    Jan Karel Pieterse
    Microsoft Excel MVP, WMVP
    www.jkp-ads.com
    Professional Office Developers Association

  5. #5
    2 Star Lounger
    Join Date
    Sep 2003
    Location
    Louisville, Kentucky, USA
    Posts
    134
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    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.

  6. #6
    2 Star Lounger
    Join Date
    Mar 2003
    Location
    London, Gtr London
    Posts
    131
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    Thanks for your input and thoughts everyone, much obliged!

  7. #7
    Silver Lounger
    Join Date
    Mar 2001
    Location
    Springfield, Ohio, USA
    Posts
    2,136
    Thanks
    0
    Thanked 1 Time in 1 Post

    Re: Improving Code (97)

    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
    <font face="Comic Sans MS">Sam Barrett, CACI </font face=comic>
    <small>And the things that you have heard... commit these to faithful men who will be able to teach others also. 2 Timothy 2:2</small>

  8. #8
    2 Star Lounger
    Join Date
    Mar 2003
    Location
    London, Gtr London
    Posts
    131
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    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

  9. #9
    Platinum Lounger
    Join Date
    Feb 2001
    Location
    Weert, Limburg, Netherlands
    Posts
    4,812
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Re: Improving Code (97)

    <hr>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<hr>
    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,......)
    Jan Karel Pieterse
    Microsoft Excel MVP, WMVP
    www.jkp-ads.com
    Professional Office Developers Association

  10. #10
    WS Lounge VIP sdckapr's Avatar
    Join Date
    Jul 2002
    Location
    Pittsburgh, Pennsylvania, USA
    Posts
    11,225
    Thanks
    14
    Thanked 342 Times in 335 Posts

    Re: Improving Code (97)

    Some good guidelines. I disagree with:

    <hr>2) Never use the Select method<hr>

    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

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •