patternMinor
Mirror and update cells across tabs and forms
Viewed 0 times
updatemirrorcellsacrossandformstabs
Problem
From the requests below I've attempted to rewrite my request. I agree completely that it would be easier for our company to provide us with help desk software. Unfortunately, my department has the assignment to sit between our end users and three separate help desks that deal with different problems. We are not allowed to enter in any help desk tickets until we have determined which of the help desk systems the issue belongs in, and many of the issues we handle are never taken to the help desk because we are able to resolve them through education. Our department manager has been working for years trying to get us access to a help desk with no result, so instead of waiting for the company to finally do something I've been working to build this file to give our team something we can use.
I have taken a copy of my file and manipulated it into a testing file which contains every cell that is affected by the VBA code but also meets our company's data security policies. I have included screenshots and the troublesome pieces of code below. Please let me know if there is any more information I can provide about the file.
Here is a picture of the priority list screen:
and here is an image of the table where the issues are stored:
The issue with the code that causes me the most trouble d/t bulk is the code that allows the users to freely update the issues from the first screen, and have that information directly updated on the second screen. The code functions completely but is very 'fat' so any help on slimming it down would be greatly appreciated.
The code for that process is:
```
Private Sub Worksheet_Change(ByVal Target As Range)
Set r1 = Sheets("Priority Table").Range("B2")
Set r2 = Sheets("Priority Table").Range("I2")
Set r3 = Sheets("Priority Table").Range("P2")
Set r4 = Sheets("Priority Table").Range("B13")
Set r5 = Sheets("Priority Table").Range("I13")
Set r6 = Sheets("Priority Table").Range("P13")
Set r7 = Sheets("Priority Table").Range("B24")
Set r8 = Sheet
I have taken a copy of my file and manipulated it into a testing file which contains every cell that is affected by the VBA code but also meets our company's data security policies. I have included screenshots and the troublesome pieces of code below. Please let me know if there is any more information I can provide about the file.
Here is a picture of the priority list screen:
and here is an image of the table where the issues are stored:
The issue with the code that causes me the most trouble d/t bulk is the code that allows the users to freely update the issues from the first screen, and have that information directly updated on the second screen. The code functions completely but is very 'fat' so any help on slimming it down would be greatly appreciated.
The code for that process is:
```
Private Sub Worksheet_Change(ByVal Target As Range)
Set r1 = Sheets("Priority Table").Range("B2")
Set r2 = Sheets("Priority Table").Range("I2")
Set r3 = Sheets("Priority Table").Range("P2")
Set r4 = Sheets("Priority Table").Range("B13")
Set r5 = Sheets("Priority Table").Range("I13")
Set r6 = Sheets("Priority Table").Range("P13")
Set r7 = Sheets("Priority Table").Range("B24")
Set r8 = Sheet
Solution
You have 12 identically-formatted tables; I would expect readable/maintainable code to read like it has some kind of concept for one of these tables. A class module would have been perfect for this. Instead we're looking at a completely cryptic sequential list of no less than 470 [undeclared] object variables, 60 of which are assigned but never used anywhere in the procedure's scope.
The procedure in question is an event handler that gets invoked everytime anything changes on that worksheet: its content should be minimal and performant, to avoid sluggishness.
The handler procedure is dereferencing the "Priority Table" and "Issue List" worksheets countless times... which is useless overhead. You could have dereferenced it once, with a
But that's not addressing the elephant in the room at all: under no circumstance is it ever a good idea to have 470 variables anywhere. Not in that procedure, not in another either. The approach is simply wrong.
Not to mention, none of these variables are actually declared: adding
I said above:
You have 12 identically-formatted tables; I would expect readable/maintainable code to read like it has some kind of concept for one of these tables.
The key here is abstraction - that's the difference between a macro-recorder script, and a well-written program.
I would start by naming things. Take one of these 12 tables, and describe it:
I'd make a class module that exposes all the above as read-write properties. Then I'd add some meta-properties so that the table object can know how to "draw" itself:
Then I'd expose some
I'd have some code to create and configure the 12 instances of that class in global scope, so that the handler can access them. And because naming variables with a numeric suffix is a code smell that basically says "you need a data structure", I'd actually be declaring 1 variable - an array that contains the 12 objects:
The
There's another elephant in the room though: all the macro does, is assign a given cell the value of another cell. 410 times.
It looks like the entire macro could be replaced with worksheet formulas doing
For example,
The procedure in question is an event handler that gets invoked everytime anything changes on that worksheet: its content should be minimal and performant, to avoid sluggishness.
The handler procedure is dereferencing the "Priority Table" and "Issue List" worksheets countless times... which is useless overhead. You could have dereferenced it once, with a
With block:With ThisWorkbook.Worksheets("Priority Table")
Set r1 = .Range("B2")
'...
End With
With ThisWorkbook.Worksheets("Issue List")
Set r221 = .Range("B2")
'...
End WithBut that's not addressing the elephant in the room at all: under no circumstance is it ever a good idea to have 470 variables anywhere. Not in that procedure, not in another either. The approach is simply wrong.
Not to mention, none of these variables are actually declared: adding
Option Explicit at the top of your module, and then getting your code to compile, is going to be quite a pain in the neck.I said above:
You have 12 identically-formatted tables; I would expect readable/maintainable code to read like it has some kind of concept for one of these tables.
The key here is abstraction - that's the difference between a macro-recorder script, and a well-written program.
I would start by naming things. Take one of these 12 tables, and describe it:
- There's a
Heading, orTitlein a merged cell at the top.
- There's a code for the
IssueSummary.
- There's a
DateReporteddate.
- There's a Boolean value to indicate whether it
HasNextFollowUp
- There's another Boolean value to indicate whether it
IsDue
- There's an
AdjustFollowUpcode. I'd make the last two tables (red headings) adhere to the format of the other tables, and move theNotesheading down to match the position of theNotesheading in every other table.
- There's a list of up to 6
ReportBackTocodes.
- There's a
PriorityCode
- There's a
TicketFieldCode
- There's a
TicketNumber
- There's a
FirstFollowUpcode
- There's a
SecondFollowUpcode
- There's a
ThirdFollowUpcode
- There's a
Notesfield
- There's an
OnTimecode that's not quite clear, that possibly refers to each follow-up codes, and theResolvedcode.
I'd make a class module that exposes all the above as read-write properties. Then I'd add some meta-properties so that the table object can know how to "draw" itself:
HeadingRowandHeadingColumn, to describe the position of the left-most cell in the table's header.
Then I'd expose some
Update method/procedure, that takes a worksheet parameter and dumps its values into that sheet - the procedure would know how to offset each value from the HeadingRow and HeadingColumn position.I'd have some code to create and configure the 12 instances of that class in global scope, so that the handler can access them. And because naming variables with a numeric suffix is a code smell that basically says "you need a data structure", I'd actually be declaring 1 variable - an array that contains the 12 objects:
Option Explicit
Public PriorityTables(1 To 12) As PriorityTable
Public Sub InitializeTable(ByVal index As Long, ByVal headerCell As Range)
Set PriorityTables(index) = New PriorityTable
With PriorityTables(index)
.HeadingRow = headerCell.Row
.HeadingColumn = headerCell.Column
.Update Sheet1
End With
End SubThe
Workbook_Open handler could conceivably call that InitializeTable procedure in a loop, so you have your 12 tables in the global array as soon as the workbook is open.There's another elephant in the room though: all the macro does, is assign a given cell the value of another cell. 410 times.
It looks like the entire macro could be replaced with worksheet formulas doing
=TheOtherSheet!ZZ42, where ZZ42 is whatever cell is supposed to be referred to.For example,
r440.Value is r220.Value, so the formula in 'Issue List'!Y11 could simply be ='Priority Table'!G39, and so on, for everything; the worksheet would keep itself up-to-date without involving any VBA code.Code Snippets
With ThisWorkbook.Worksheets("Priority Table")
Set r1 = .Range("B2")
'...
End With
With ThisWorkbook.Worksheets("Issue List")
Set r221 = .Range("B2")
'...
End WithOption Explicit
Public PriorityTables(1 To 12) As PriorityTable
Public Sub InitializeTable(ByVal index As Long, ByVal headerCell As Range)
Set PriorityTables(index) = New PriorityTable
With PriorityTables(index)
.HeadingRow = headerCell.Row
.HeadingColumn = headerCell.Column
.Update Sheet1
End With
End SubContext
StackExchange Code Review Q#155987, answer score: 4
Revisions (0)
No revisions yet.