Page 1 of 2 12 LastLast
Results 1 to 15 of 22
  1. #1
    Lounger
    Join Date
    Jul 2014
    Posts
    33
    Thanks
    7
    Thanked 3 Times in 3 Posts

    Trimming VBA Code

    Hi all!

    Just a quicky really, as I'm (slowly) getting to grips with VBA I'm realising that a lot of the code I've recorded/edited could probably be trimmed down a lot..

    Here's a snippet of a code I use to pull data from other sheets, I'll post for one sheet as essentially it's the same code 30 times over with different file names.

    Code:
        Workbooks.Open Filename:= _
            "301-LBG QueueGroupPerformancebyPeriod.xls"
            Range("C8:C56").Copy
        Windows("Daily Service Level Scorecard Macro Testing.xlsm").Activate
            Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        Windows("301-LBG QueueGroupPerformancebyPeriod.xls").Activate
            Range("T8:T56").Copy
        Windows("Daily Service Level Scorecard Macro Testing.xlsm").Activate
            Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        Windows("301-LBG QueueGroupPerformancebyPeriod.xls").Activate
            Range("F56").Copy
        Windows("Daily Service Level Scorecard Macro Testing.xlsm").Activate
            Sheets("Abandon Count").Range("B2").PasteSpecial Paste:=xlPasteValues
        Windows("301-LBG QueueGroupPerformancebyPeriod.xls").Close
        'Updated with Abandon Count
    Does anyone have an idea how I can reduce the lines of code ( if it's possible of course! ). It doesn't really matter for any reason, but I'm a big fan of elegance and I'd like to make my codes as tight as possible, also it might make them easier to edit!

    Thanks

    R

  2. #2
    Super Moderator RetiredGeek's Avatar
    Join Date
    Mar 2004
    Location
    Manning, South Carolina
    Posts
    9,433
    Thanks
    371
    Thanked 1,456 Times in 1,325 Posts
    Rathril,
    One thing you can do is use object variables which will at least make the code easier to read and should be more efficient.

    Code:
        Dim wkbDSL as Workbook
        Dim wkbQGP as Workbook
        
        Set wkbDSL = ActiveWorkbook
        Set wkbQGP = Workbooks.Open( Filename:= _
            "301-LBG QueueGroupPerformancebyPeriod.xls")
    
     '*** QGP last opened so Active
        Range("C8:C56").Copy
        wkb.DSL.Activate
            Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wkbQGP.Activate
            Range("T8:T56").Copy
        wkbDSL.Activate
            Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        wkbQGP.Activate
            Range("F56").Copy
        wkvDSL.Activate
            Sheets("Abandon Count").Range("B2").PasteSpecial Paste:=xlPasteValues
       wkbQGP.Close
        'Updated with Abandon Count
    There may be more to do but I'm working w/bad hand making typing & testing hard. Note: untested code test on copy!!!!! I think I maintained you logic but please check I'm sure you can follow & fix if necessary/

    HTH
    May the Forces of good computing be with you!

    RG

    PowerShell & VBA Rule!

    My Systems: Desktop Specs
    Laptop Specs

  3. The Following User Says Thank You to RetiredGeek For This Useful Post:

    Rathril (2014-10-14)

  4. #3
    Lounger
    Join Date
    Jul 2014
    Posts
    33
    Thanks
    7
    Thanked 3 Times in 3 Posts
    Ohh that would definitely make it look a lot neater plus get me used to setting stuff as variables!

    I'll have have a test with one file and let you know how it goes

    Thanks a lot

    R

    Editted this since it doesn't really need a new post, there was a couple of typos but once I debugged them it worked perfectly. I'll update the macro tomorrow at work when I get paid for it though :P

    Thanks again!
    Last edited by Rathril; 2014-10-14 at 16:04.

  5. #4
    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
    Another suggestion is not to activate anything, but use explicit references
    Code:
        Dim wkbDSL as Workbook
        Dim wkbQGP as Workbook
        
        Set wkbDSL = ActiveWorkbook
        Set wkbQGP = Workbooks.Open( Filename:= _
            "301-LBG QueueGroupPerformancebyPeriod.xls")
    
        wkbQGP.Range("C8:C56").Copy
        wkb.DSL.Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wkbQGP.Range("T8:T56").Copy
        wkbDSL.Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wkbQGP.Range("F56").Copy
        wkvDSL.Sheets("Abandon Count").Range("B2").PasteSpecial Paste:=xlPasteValues
    
       wkbQGP.Close
        'Updated with Abandon Count

  6. #5
    Lounger
    Join Date
    Jul 2014
    Posts
    33
    Thanks
    7
    Thanked 3 Times in 3 Posts
    Hi sdckapr thanks for your post! I tried the code you posted and it brings up the error
    "Run-time error '438':
    Object doesn't support this property or method"

    Not too great on the whole debugging yet, but I'm using excel 2010 if that has any bearings!

  7. #6
    Super Moderator RetiredGeek's Avatar
    Join Date
    Mar 2004
    Location
    Manning, South Carolina
    Posts
    9,433
    Thanks
    371
    Thanked 1,456 Times in 1,325 Posts
    Quote Originally Posted by Rathril View Post
    a couple of typos but once I debugged them it worked perfectly. I'll update the macro tomorrow at work when I get paid for it though :P
    Sorry about that I'm working with a splint on my left hand..tendonitis and arthritis or simply old man disease!

    I think the error with Steve's code is that paste only works on the active worksheet, but I could be mis-remembering.

    HTH
    May the Forces of good computing be with you!

    RG

    PowerShell & VBA Rule!

    My Systems: Desktop Specs
    Laptop Specs

  8. #7
    WS Lounge VIP rory's Avatar
    Join Date
    Dec 2000
    Location
    Burwash, East Sussex, United Kingdom
    Posts
    6,280
    Thanks
    3
    Thanked 191 Times in 177 Posts
    The problem is trying to apply Range to a Workbook object rather than worksheet:

    Code:
        Dim wkbDSL as Workbook
        Dim wkbQGP as Workbook
        
        Set wkbDSL = ActiveWorkbook
        Set wkbQGP = Workbooks.Open( Filename:= _
            "301-LBG QueueGroupPerformancebyPeriod.xls")
    
        wkbQGP.Activesheet.Range("C8:C56").Copy
        wkb.DSL.Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wkbQGP.Activesheet.Range("T8:T56").Copy
        wkbDSL.Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wkbQGP.Activesheet.Range("F56").Copy
        wkvDSL.Sheets("Abandon Count").Range("B2").PasteSpecial Paste:=xlPasteValues
    
       wkbQGP.Close
        'Updated with Abandon Count
    for example.
    Regards,
    Rory

    Microsoft MVP - Excel

  9. #8
    Lounger
    Join Date
    Jul 2014
    Posts
    33
    Thanks
    7
    Thanked 3 Times in 3 Posts
    Thanks Rory - I'll try with the updated code, if that works i will have saved about 150 lines of code from that suggestion!

    And @RetiredGeek - Using the format you suggested means that it will be a lot easier to set up the report for other people to run without giving them access to my folders... should save me a lot of time! Thanks a lot

  10. #9
    Gold Lounger Maudibe's Avatar
    Join Date
    Aug 2010
    Location
    Pa, USA
    Posts
    2,629
    Thanks
    114
    Thanked 645 Times in 589 Posts
    A few things to shorten the code further:
    1. After opening the source workbook, once you reactivate the main workbook (ThisWorkbook) you do not have to activate it each time you copy from the source workbook. There is no need to Dim and set the ActiveWorkbook.
    2. Use the with statement to refer to the source workbook
    3. Making each line a complete copy/paste function shortens the code and makes it easier to follow

    HTH,
    Maud

    Code:
        Dim Source As Workbook
        Set Source = Workbooks.Open(Filename:="301-LBG QueueGroupPerformancebyPeriod.xls")
        ThisWorkbook.Activate
        With Source.ActiveSheet
        .Range("C8:C56").Copy: Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        .Range("T8:T56").Copy: Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        .Range("F56").Copy: Sheets("Abandon Count").Activate: Range("B2").PasteSpecial Paste:=xlPasteValues
        Source.Close
        End With

  11. #10
    Super Moderator RetiredGeek's Avatar
    Join Date
    Mar 2004
    Location
    Manning, South Carolina
    Posts
    9,433
    Thanks
    371
    Thanked 1,456 Times in 1,325 Posts
    Hey Y'all,

    [Disclaimer]This is My Opinion based on what I what I was taught Years and Years ago[/Disclaimer]

    It has been generally accepted coding principle that readability trumps shortening the code. The compiler will ignore white space anyway and the little bit of storage in insignificant.

    That said I would prefer Rory's solution to Maud's. They will both work just fine! However, you generally don't want to put multiple statements on a single line (use of the : operator) as this can easily be over looked when you look at the code months later or misunderstood by someone else not used to using this operator. The same applies to the ThisWorkbook object vs clearly enumerating which workbook. If you have several workbooks open in a complex setup you'll always have to sit back and think which workbook is THIS workbook? Note: this can also apply to ActiveWorkbook and ActiveCell (yes, I'm guilty of using both headbang.gif)

    My bottom line: let the computer do the work and make the code as easy as possible for us mere humans to understand. YMMV
    Last edited by RetiredGeek; 2014-10-19 at 08:22.
    May the Forces of good computing be with you!

    RG

    PowerShell & VBA Rule!

    My Systems: Desktop Specs
    Laptop Specs

  12. #11
    WS Lounge VIP access-mdb's Avatar
    Join Date
    Dec 2009
    Location
    Oxfordshire, UK
    Posts
    1,721
    Thanks
    146
    Thanked 156 Times in 149 Posts
    I agree RG. Many years ago, I was programming in Fortran 66. There was one way to assign values to array elements that saved a couple of keystrokes. If you had an array e.g. somename(10,10), you could make somename(11) equal some value and the array element (2,10) would have that value. It was an undocumented feature. However, when someone else looked at the code, they wouldn't know what it meant. What was worse, when we upgraded to Fortran 77, that little undocumented feature stopped working! And programs then failed! And some poor programmer had to work out why.

    The moral of all this is KISS. The computer can cope with all sorts. we can't. So make it as easy for poor old humans as you can.

    I've no doubt that many programmers on here, have wondered how some code works - and they wrote it only a few weeks previously!

    I would ask the question: are any of the methods above faster than the others? I suspect that the answer is, only marginally at best (unless the code is many thousands or millions of lines and then it's a whole different ball game).

  13. #12
    Gold Lounger Maudibe's Avatar
    Join Date
    Aug 2010
    Location
    Pa, USA
    Posts
    2,629
    Thanks
    114
    Thanked 645 Times in 589 Posts
    For Rory's code to work, the lines:

    Code:
    wkbDSL.Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply 
    
    need to be changed to
    
    wkbDSL.ActiveSheet.Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    If one feels that:

    Code:
    Dim wkbDSL as Workbook
    Set wkbDSL = ActiveWorkbook
    wkbDSL.Activate
    
    Is preferable and more clear than:
    
    ThisWorkbook.Activate
    And if you want to repeatedly make the current workbook the active workbook when it already is, then I don't have a problem with that.

    If one wants to explicitly refer to the active workbook by name when it is not necessary, I don't have a problem with that as well.

    If one wants to explicitly refer to the source workbook (wkbQGP) by name each instance instead of using the With Statement, that's cool too.

    For a simple copy/paste one liner, if someone prefers a complete action to be performed in vertical block code instead of one line separated by ":" then so be it.

    But the OP's request was:
    Does anyone have an idea how I can reduce the lines of code ( if it's possible of course! ).
    I think that's what we all did to some extent, but as they stand, minus a couple of typos, RG's and my code are the only ones that will function correctly if you actually test them.

  14. #13
    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
    In my experience, since code can be run without .Activate and .Select, it is poor practice to use them Using them complexity to the code, slows it down, and can get disconcerting to the user if the active location at the end of the coding is different than it was before.

    If multiple worksheets and/or workbooks are being manipulated, I find it good practice to create objects for them so it is clear in the code what you are referring to. It makes it clearer what the code is working on. This can be done using a WITH or not (a WITH.. END WITH is just shorthand explicit). Bu the objects do not have to be explicitly named, they can use Activesheet and this workbook using even something like:

    dim wksAct as worksheet
    set wksAct = activesheet

    Using implicit (which assumes that the activesheet/ activeworkbook) can be fine, but can be problematic. If you active and/or select in the code, you have to make sure you keep track of what sheet/book is active all the time and it can be problematic: opening new files makes a new active sheet for example. it can also be problematic when debugging code. For example, if you step through the code, and it is not running, if you examine the sheets/books, and change which is active, and don't change it back, your code will not work as desired.

    ThisWorkbook does not change when the code is run (it refers to the workbook containing the code). But Activeworkbook and activesheet, can change depending on when the code is run and how the code is called and is not always constant throughout the process. It can be confusing when debugging if one part of the code expects the activesheet to be thisworkbook.Worksheets(1) and in another part the code the activesheet is Workbooks(2).Worksheets(3).

    Steve

  15. #14
    WS Lounge VIP rory's Avatar
    Join Date
    Dec 2000
    Location
    Burwash, East Sussex, United Kingdom
    Posts
    6,280
    Thanks
    3
    Thanked 191 Times in 177 Posts
    It is true that having pointed out the problem with Steve's original posted code, I then repeated it!

    I would use something like this (though I'd prefer sheet names to assuming the correct sheet is active):

    Code:
        Dim wsTo                  As Worksheet
        Dim wsFrom                As Worksheet
    
        Set wsTo = ActiveSheet
        Set wsFrom = Workbooks.Open(Filename:="301-LBG QueueGroupPerformancebyPeriod.xls").ActiveSheet
    
        wsFrom.Range("C8:C56").Copy
        wsTo.Range("B3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
    
        wsFrom.Range("T8:T56").Copy
        wsTo.Range("C3").PasteSpecial Paste:=xlPasteValues, Operation:=xlMultiply
        wsFrom.Range("F56").Copy
        wsTo.Range("B2").PasteSpecial Paste:=xlPasteValues
    
        wsFrom.Parent.Close
        'Updated with Abandon Count
    As a wise man once said:
    Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. Code for readability.
    Regards,
    Rory

    Microsoft MVP - Excel

  16. The Following User Says Thank You to rory For This Useful Post:

    zeddy (2014-10-21)

  17. #15
    WS Lounge VIP
    Join Date
    Mar 2002
    Location
    Newcazzle, UK
    Posts
    2,819
    Thanks
    133
    Thanked 480 Times in 457 Posts
    Hi Rory

    ..your wise man advice is the best I've seen on this site. I am preparing to move house now.

    re post #2
    RG - I was once caught out in vba by assuming the last opened Excel file was the Active file. It isn't always. It depends on how that particular Excel file was last saved (e.g. if it was saved while in a 'minimised' state)

    zeddy

Page 1 of 2 12 LastLast

Posting Permissions

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