3
\$\begingroup\$

I have a section of code that I've been attempting to clean up and hopefully speed up. The section of code where I pass values in order to generate a key and update a table on a different sheet I think is my biggest problem. However I would like any suggestions/modifications to clean this section up first.

An overview is my worksheet has a header section where the user will update the monthly sales,units or avg. cost field. Once this is done I need to recalculate all values in that month. This is anywhere from 10 to 1200 rows. As the update executes the following occurs:

  1. Cell is updated with the impact of the header change.
  2. A new unique key is built.
  3. The new key is found in a table and the change value is inserted.

I was stuck manually changing the "ColumnCount" variable to maintain the offset in the proper position. Any help would be great.

'Table update afet net units header update
Dim i As Integer
'LastRow used to determine final used row on worksheet
    LastSourceRow = LastRow(destsheet, "B")
'Column Count initially set to -2 to offset target to desired cell
    ColumnCount = -2
    RowCount = 1

'Runs from row 1 to the last row on the sheet
For RowCount = 1 To LastSourceRow Step 5
    i = 5
    'After header change offset 5 rows and 2 columns back to capture cell change

    Set target = target.Offset(i, ColumnCount)

        'Running Status
        Application.StatusBar = "Target value change : " & target.Row & " of " & LastSourceRow & " " & Format(target.Row / LastSourceRow, "Percent")

            'If change cell is 0 then skip and sel column count to 0 to keep offset aligned
            If target = 0 Then

            'Do Nothing if 0 value
            ColumnCount = 0
            Else

                'Generate unique key and pass to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'offset target by 1 to capture change
                ColumnCount = 1

                'Set target offset to new cell
                Set target = target.Offset(, ColumnCount)

                'Generate key and pass target offset value to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'Set target offset back to original cell
                ColumnCount = -1

                Set target = target.Offset(, ColumnCount)
                'Set column count to 0 to maintain alignment when loop repeats

                ColumnCount = 0
            End If
Next RowCount
\$\endgroup\$
  • 3
    \$\begingroup\$ I strongly recommend editing the title to at least hint about what the code is doing - not what you want out of a review. \$\endgroup\$ – Mathieu Guindon Feb 24 '15 at 22:02
  • \$\begingroup\$ I took a stab at the title and made formatted your list as a list. Feel free to further improve the title. \$\endgroup\$ – RubberDuck Feb 24 '15 at 22:05
1
\$\begingroup\$

I'm doing a little guess work here because you've not shown us the method'd signature. I apologize if I've made some bad assumptions.

Typically, when I see a Target Range, it means we're working inside of a worksheet's OnChange event. Assuming that, I would recommend introducing a new Range variable into the mix so you're not accidentally modifying the incoming range. Adding an extra variable would make it clear and explicit when/if you're programmatically changing the same range the user modified.

This snippet makes me believe it's the right thing to do even if my assumption is wrong.

                'Set target offset to new cell
                Set target = target.Offset(, ColumnCount)

                'Generate key and pass target offset value to table update module
                modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

                'Set target offset back to original cell
                ColumnCount = -1

                Set target = target.Offset(, ColumnCount)
                'Set column count to 0 to maintain alignment when loop repeats

By introducing an offsetTarget variable, you should be able to avoid tracking the column count and all this setting and resetting because you've never modified target to begin with.

The other thing I would be careful of is detailed at the end of this answer and again assumes this code resides in the Worksheet_Change event. Target could possibly be a multi-cell Range. Double check your code to make sure it can deal with that edge case.

One last thing. This method takes in an awful lot of parameters.

modTableUpdate.TableUpdate target, destsheet, sourcesheet, ColumnHeader, ColumnFormula, Celladdress, Key, cell, result

Consider creating a simple class to encapsulate that data and simplify the call.

modTableUpdate.TableUpdate instanceOfYourAwesomeNewClass
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.