Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow this question on Stack Overflow.)

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow.)

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow.)

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow this question on Stack Overflow.)

To speed up things even somewhat more, you can read the entire range you like to copy into an array via Value2 and then process it. You can also set entire ranges from arrays by setting the Value property of a range. Using arrays is the fastest thing you can do to copy large ranges. (See this blog post blog post testing the various ways to copy the values in ranges in Excel.)

  1. The use of Target in the procedure, or at least its name, is rather confusing. Apparently, it is only used in the condition that is tested and it is never used as a target to copy or the like. It seems as if the check might belong into another procedure calling this one.

  2. OurYour code will throw an error whenever Sheet(CE1 Data) is not the active sheet. The problem is that in:

     Set rngQuantityCells = Sheets(CE1 Data).Range(B120, Range(B120).End(xlDown))
    

the second Range is not qualified, i.e. it is always implicitly referring to the active sheet. It would be a good practice to always qualify calls to Range, even if itit's from ActiveSheet.

  1. If the sheet Sheet(CE1 Data) is not active, the paste range will not be inside the cleared range because the paste range will be on the active sheet. (This obviously only applies after qualifying the offending Range from the second comment.)

  2. You might want to save the the worksheet in a variable and not query the Sheets collection all the time. Personally, I would even inject the worksheet to use as a parameter of type Worksheet.

  3. Given that you apparently only use this procedure from the worksheet Sheet(CE1 Data), you might consider using a With statement to access the sheet.

  4. You are using quite a number of magic numbers in the procedure, like the bounds in which Target may be or the explicit ranges. It might be better to use constants or named ranges. If you ever want to change from where this procedure reads or to where it writes, named ranges would make your life easier.

One more thing.: Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target.

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow.)

To speed up things even somewhat more, you can read the entire range you like to copy into an array via Value2 and then process it. You can also set entire ranges from arrays by setting the Value property of a range. Using arrays is the fastest thing you can do to copy large ranges. (See this blog post testing the various ways to copy the values in ranges in Excel.)

  1. The use of Target in the procedure, or at least its name, is rather confusing. Apparently, it is only used in the condition that is tested and it is never used as a target to copy or the like. It seems as if the check might belong into another procedure calling this one.

  2. Our code will throw an error whenever Sheet(CE1 Data) is not the active sheet. The problem is that in:

     Set rngQuantityCells = Sheets(CE1 Data).Range(B120, Range(B120).End(xlDown))
    

the second Range is not qualified, i.e. it is always implicitly referring to the active sheet. It would be a good practice to always qualify calls to Range, even if it from ActiveSheet.

  1. If the sheet Sheet(CE1 Data) is not active, the paste range will not be inside the cleared range because the paste range will be on the active sheet. (This obviously only applies after qualifying the offending Range from the second comment.)

  2. You might want to save the the worksheet in a variable and not query the Sheets collection all the time. Personally, I would even inject the worksheet to use as a parameter of type Worksheet.

  3. Given that you apparently only use this procedure from the worksheet Sheet(CE1 Data), you might consider using a With statement to access the sheet.

  4. You are using quite a number of magic numbers in the procedure, like the bounds in which Target may be or the explicit ranges. It might be better to use constants or named ranges. If you ever want to change from where this procedure reads or to where it writes, named ranges would make your life easier.

One more thing. Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target.

From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value or Value2 property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow.)

To speed up things even somewhat more, you can read the entire range you like to copy into an array via Value2 and then process it. You can also set entire ranges from arrays by setting the Value property of a range. Using arrays is the fastest thing you can do to copy large ranges. (See this blog post testing the various ways to copy the values in ranges in Excel.)

  1. The use of Target in the procedure, or at least its name, is rather confusing. Apparently, it is only used in the condition that is tested and it is never used as a target to copy or the like. It seems as if the check might belong into another procedure calling this one.

  2. Your code will throw an error whenever Sheet(CE1 Data) is not the active sheet. The problem is that in:

     Set rngQuantityCells = Sheets(CE1 Data).Range(B120, Range(B120).End(xlDown))
    

the second Range is not qualified, i.e. it is always implicitly referring to the active sheet. It would be a good practice to always qualify calls to Range, even if it's from ActiveSheet.

  1. If the sheet Sheet(CE1 Data) is not active, the paste range will not be inside the cleared range because the paste range will be on the active sheet. (This obviously only applies after qualifying the offending Range from the second comment.)

  2. You might want to save the worksheet in a variable and not query the Sheets collection all the time. Personally, I would even inject the worksheet to use as a parameter of type Worksheet.

  3. Given that you apparently only use this procedure from the worksheet Sheet(CE1 Data), you might consider using a With statement to access the sheet.

  4. You are using quite a number of magic numbers in the procedure, like the bounds in which Target may be or the explicit ranges. It might be better to use constants or named ranges. If you ever want to change from where this procedure reads or to where it writes, named ranges would make your life easier.

One more thing: Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target.

added 1157 characters in body
Source Link
M.Doerner
  • 1.5k
  • 10
  • 12

EDIT:

I did not really realize that you wrote a handler for the worksheet changed event. So, the parameter Target has to be there. However, it would be good practice to separate the two current responsibilities of your handler, to decide what kind of thing to do on a worksheet changed event and to copy over the cells. This can be achieved by extracting everything inside the conditional into its own sub, which preferably should not depend on the active sheet. Then my comments above apply to that sub.

One more thing. Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target.

Considering that Target may contain more than one cell, you might also want to consider whether your condition on Target is correct in all circumstances.

EDIT:

I did not really realize that you wrote a handler for the worksheet changed event. So, the parameter Target has to be there. However, it would be good practice to separate the two current responsibilities of your handler, to decide what kind of thing to do on a worksheet changed event and to copy over the cells. This can be achieved by extracting everything inside the conditional into its own sub, which preferably should not depend on the active sheet. Then my comments above apply to that sub.

One more thing. Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target.

Considering that Target may contain more than one cell, you might also want to consider whether your condition on Target is correct in all circumstances.

Used a numbered list rather than word. Should improve readability
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
Used a numbered list rather than word. Should improve readability
Source Link
Loading
Source Link
M.Doerner
  • 1.5k
  • 10
  • 12
Loading
lang-vb

AltStyle によって変換されたページ (->オリジナル) /