HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Writing to many cells from many controls' respective value

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
controlswritingrespectivevaluecellsmanyfrom

Problem

I have this program on Excel VBA, that implements a userform in order to simplify input and to create control charts from the data input analyze the data and send emails if certain conditions are met.

My problem is that everything works properly but after running line by line I noticed that when filling up the corresponding cells in excel with the user input on the userform the macro takes long to execute each line.

I am wondering if any of you have a different method of doing this that would reduce the time it takes to run through all these lines.

```
Function completeData(mo As String)

Dim row As Integer
row = moRow(mo)
DataHistory

'Tag Information
Worksheets("Data").Cells(row, 2).value = ComboBox1.value 'set UPC
Worksheets("Data").Cells(row, 4).value = TextBox2.value 'set DateCured
Worksheets("Data").Cells(row, 5).value = TextBox48.value 'set DateTested
Worksheets("Data").Cells(row, 41).value = ComboBox2.value 'set Grinder
Worksheets("Data").Cells(row, 42).value = ComboBox3.value 'set Metal
Worksheets("Data").Cells(row, 43).value = ComboBox4.value 'set Press
Worksheets("Data").Cells(row, 44).value = ComboBox5.value 'set Clock
Worksheets("Data").Cells(row, 45).value = ComboBox6.value 'set Oven

'weight 0min Wheels
Worksheets("Data").Cells(row, 15).value = TextBox3.value
Worksheets("Data").Cells(row + 1, 15).value = TextBox4.value
Worksheets("Data").Cells(row + 2, 15).value = TextBox5.value

'Before Thikness
''''FirstWheel
Worksheets("Data").Cells(row, 6).value = TextBox6.value
Worksheets("Data").Cells(row, 7).value = TextBox7.value
Worksheets("Data").Cells(row, 8).value = TextBox8.value
Worksheets("Data").Cells(row, 9).value = TextBox9.value
''''SecondWheel
Worksheets("Data").Cells(row + 1, 6).value = TextBox10.value
Worksheets("Data").Cells(row + 1, 7).value = TextBox11.value
Worksheets("Data").Cells(row + 1, 8).value = TextBox12.value
Worksheets("Data").Cells(row + 1, 9).value = TextBox13.value
''''ThirdWheel
Worksheets("Data").Cells(row + 2, 6).value = Tex

Solution

The first thing to do is to switch off calculation, sheet events and screen updating before you write anything to a worksheet, and restore it when you're done: that way you won't have Excel try to re-calculate the entire workbook whenever you write to a cell.

Next, name things, starting with the "Data" sheet. Select the worksheet in the Project Explorer (Ctrl+R), then bring up the Properties toolwindow (F4) and type DataSheet under the sheet's (name) property.

Then you can do this:

With DataSheet
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    ...
End With


In the form's designer (F7), give each control a meaningful name. For example ComboBox1 contains a UPC code, so it should be named SelectedUPC.

The problem with this is that you already have tons of code referring to these controls, and renaming ComboBox1 to SelectUPC would break your code, because ComboBox1 would be left referring to nothing.

You'll need tools to do this cleanly. Fortunately Rubberduck can help with this refactoring:

You could right-click on a control, select Rubberduck > Rename (or right-click a ComboBox1 identifier reference in the code, select Rubberduck > Refactor > Rename), then specify SelectedUPC as the new name, and then...

Rubberduck renames the associated event handler(s) and in-code identifier references for you - so there's no excuse for having meaningless identifiers anywhere anymore.


DISCLAIMER I'm heavily involved with the Rubberduck project.

Speaking of naming, it's not clear at all what moRow means to be doing, nor who is calling that function, or even why it needs to be a Function.

Procedures that don't return a value should be Sub procedures; a Sub procedure executes a sequence of operations; a Function procedure computes a value and returns it to the caller.

Also, module members in VBA are implicitly Public, which means completeData (which should be CompleteData to be PascalCase like all other VBA procedures) is a public member of that UserForm.

I'd strongly recommend reading up on VBA UserForm best practices, and restructure your code so that the form doesn't "run the show": the form should be there only to provide the rest of the code with user input, not to actually perform the worksheet manipulations - a form's code-behind should only be concerned about the form and its controls; a form that knows about a worksheet, plainly knows (and does) too many things.

I'd also suggest you define an enum to name each column of your "Data" sheet, so instead of this:

With DataSheet
    .Cells(row, 2).Value = SelectedUPC.Value


You would have that:

With DataSheet
    .Cells(row, DataSheetColumns.UPC).Value = SelectedUPC.Value


All that's needed is a simple declaration:

Private Enum DataSheetColumns
    UPC = 2
    DateCured = 4
    DateTested = 5
    ...
End Enum


That makes it much easier to read the rest of the code, and seeing DataSheetColumns.UPC being assigned to SelectedClock.Value looks much more wrong than if it's column 44 being assigned: making wrong code look wrong is even more important than writing code that "works".

Code Snippets

With DataSheet
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    .Cells(...).Value = ...
    ...
End With
With DataSheet
    .Cells(row, 2).Value = SelectedUPC.Value
With DataSheet
    .Cells(row, DataSheetColumns.UPC).Value = SelectedUPC.Value
Private Enum DataSheetColumns
    UPC = 2
    DateCured = 4
    DateTested = 5
    ...
End Enum

Context

StackExchange Code Review Q#139373, answer score: 5

Revisions (0)

No revisions yet.