0

I am trying to optimise some data sorting code, the original code looks like it has been "written" through the use of excels "Record Macro" function.

The following is an example from the code.

Range("A12").Offset(z, 0).Select
Selec.Resize(Selection.Rows.Count, Selection.Columns.Count + (X2 - 1)).Select
Selection.Copy
Range("C4").Offset(y, 0).Select
ActiveSheet.Paste

In order to make this code more efficient, should i re-write this section to involve a range variable that its' .Value is equal to the Selection data?

The X2, z and y variables are used as part of the copy function and are slowly increased to make reading the end product much easier. They are altered elsewhere in the module.

I would just like to see some suggestions for what you think I should do.

I have read the very popular "How to avoid using Select in Excel VBA Macros" question on here, and am just looking for some further advice.

(How to avoid using Select in Excel VBA macros)

Thanks

Community
  • 1
  • 1
dyslexicgruffalo
  • 364
  • 1
  • 4
  • 18
  • That would be: `Range("A12").Offset(z, 0).Resize(1, X2).Copy Destination:=Range("C4").Offset(y, 0)` – Rory Jul 08 '16 at 13:57
  • Thankyou this seems to work! I shall set to work doing similar things to the rest. Thanks – dyslexicgruffalo Jul 08 '16 at 14:02
  • Hiya @Rory i dont suppose you could show me how you could do this one? Its just taking me a bit of time to learn how you are converting it - Range("A12").Offset(z, 0).Select Selection.Resize(Selection.Rows.Count, Selection.Columns.Count + (X2 - x - 1)).Select Selection.Copy Range("C4").Offset(y, X2 - a).Select ActiveSheet.Paste – dyslexicgruffalo Jul 08 '16 at 14:16
  • Sure: `Range("A12").Offset(z, 0).Resize(1, X2 - x).Copy Destination:=Range("C4").Offset(y, X2 - a)` – Rory Jul 08 '16 at 14:21
  • Your comment would be `Cells(12 + z, 1).Resize(, X2 - x).Copy Cells(4 + y, 3 + X2 - a)` (one line).... I was to slow... still... there is no need for the `Destination:=` – Dirk Reichel Jul 08 '16 at 14:22

2 Answers2

0

Copy/pasting isn't very efficient. Doing Range("X1").value = Range("Y1").value is better. This is what I would do 

'change this
Selection.Copy
Range("C4").Offset(y, 0).Select
ActiveSheet.Paste

'to this
Range("C4").Offset(y, 0).Value = Selection.Value 'selection value is the range that is being copied.
Alex
  • 87
  • 1
  • 6
  • So you wouldnt advise me re-writing it to avoid using selection or select at all? – dyslexicgruffalo Jul 08 '16 at 13:56
  • If it's not broke, don't fix it! However, the copy/paste thing isn't very efficient, and depending what you're doing, you may not want something that is going to utilize/modify your clipboard. – Alex Jul 08 '16 at 13:58
  • I completely agree with the if it aint broke dont fix it ideology, however the file sizes this spreadsheet has to sift through are massive and sometimes the user can be sitting there for 3 minutes while excel works it all out. (that isnt the only selection/select/copy/paste section there are about 15. Each are differrent parts of an if or case statement! – dyslexicgruffalo Jul 08 '16 at 14:01
  • There isn't a whole lot I can recommend without seeing the rest of the code, but I get if it's sensitive work info. There are a lot of ways to make VBA code more efficient. A good way is to add "Application.Screenupdating = False" at the beginning of your Sub and "Application.Screenupdating = True" at the end. If you're working with loops and it freezes up, a good solution is to add a "DoEvents" somewhere in the loop. This doesn't help runtime, but it'll help it not lock. Check this out: http://datapigtechnologies.com/blog/index.php/ten-things-you-can-do-to-speed-up-your-excel-vba-code/ – Alex Jul 08 '16 at 14:04
  • Yeah sorry i would post more but it is work sensitive and lots of it is another person's work, i know about updating the screen. The program doesnt need DoEvents as it just reads the text from another file. But thank you for your help! – dyslexicgruffalo Jul 08 '16 at 14:12
0

I guess this would be the easiest way. 

Range("C4").Offset(y, 0).Resize(1, x2) = Range("A12").Offset(z, 0).Resize(1, x2)

Although I would advise:

  • to avoid .Offset and work with named ranges instead
  • to avoid .Copy and .Paste sometimes your data might be too big for your clipboard and using just the =-operator is way faster
  • to not name your variables with capital letters or additional ciphers, it can be very confusing
  • use this format .Cells( row, column ) instead of `.Cells("A1"). It's easier to handle and can be changed more easily via calculations

HTH

Tom K.
  • 1,020
  • 1
  • 12
  • 28
  • Except you need to resize the range on the left of the `=` so it's the same size as the one on the right. Also, you should specify the `Value` or `Value2` property and not assume defaults. – Rory Jul 08 '16 at 14:23
  • @Rory, wouldnt VBA just use the `.offset(y, 0)` as a target cell then everything lower and to the right would be put in? – dyslexicgruffalo Jul 08 '16 at 14:26
  • @Rory actually that wouldnt matter anyway as i am just copying one Row - thanks for the thought tho – dyslexicgruffalo Jul 08 '16 at 14:28
  • @lewisthegruffalo No - that only works with copy/paste. Assigning values requires you to do the resizing yourself. You still need to resize to pass all the columns. – Rory Jul 08 '16 at 14:28
  • @Rory so i would just copy the `.Resize(1, x2)` to the left hand side of the `=`? – dyslexicgruffalo Jul 08 '16 at 14:30
  • @lewisthegruffalo Yes, or use the copy/paste. If you only want values, this is fine. – Rory Jul 08 '16 at 14:32
  • incase other people look at this post i shall say that i am not going to use `.cells()` as i am copying to more than one cell (and i trust range more) :P – dyslexicgruffalo Jul 08 '16 at 14:43
  • edited it @Rory, as for the `.Cells`, you can define `Ranges` like this: `Range(Cells(1,1), Cells(2,2))` – Tom K. Jul 08 '16 at 15:00