PDA

View Full Version : Solved: Misserable example....



Papadopoulos
04-09-2010, 03:42 PM
This is a small snippet of the mess I have created.
I am hoping that you can help me improve how this is done (This is actually one of the shorter examples of the nightmare that I have created and now need to clean up)

In this case, I am using a drop down to reference a list from another sheet. Based on the selection I fill in a couple of cells of information.
So far I have done this through a series of if/then statments (16 for this one alone!) which means that anytime the list changes...
Not pretty I know.


Sub SheetSize()
'sets width, height, and number up based on choice - 16 option s
If Worksheets("Sheet1").Range("szIND").Value = 1 Then
Worksheets("ref").Range("sizeDesc").Value = Worksheets("sizeRef").Range("A2")
Worksheets("Sheet1").Range("Width").Value = Worksheets("sizeRef").Range("B2")
Worksheets("Sheet1").Range("Height").Value = Worksheets("sizeRef").Range("C2")
Worksheets("Sheet1").Range("qUP").Value = Worksheets("sizeRef").Range("D2")
Worksheets("ref").Range("cutsPer").Value = Worksheets("sizeRef").Range("E2")
' Remove macro from bleed check box and enter null val for the bleedVal cell
' that is used in the quote and hide the check box
ActiveSheet.Shapes("Check Box 47").OnAction = ""
ActiveSheet.Shapes("Check Box 47").Visible = False
Worksheets("Sheet1").Range("bleedVal").Value = ""
' End checkbox manipulation
' Perform the same for the imposition button
ActiveSheet.Shapes("imposeCalc").OnAction = ""
ActiveSheet.Shapes("imposeCalc").Visible = False
' End interface manipulations
Range("forms").Select
ElseIf Worksheets("Sheet1").Range("szIND").Value = 2 Then
Worksheets("ref").Range("sizeDesc").Value = Worksheets("sizeRef").Range("A3")


Thanks,
David
I am attaching the file

Bob Phillips
04-09-2010, 04:12 PM
Normally, when you have code that is repeated, like this bit




' Remove macro from bleed check box and enter null val for the bleedVal cell
' that is used in the quote and hide the check box
ActiveSheet.Shapes("Check Box 47").OnAction = ""
ActiveSheet.Shapes("Check Box 47").Visible = False
Worksheets("Sheet1").Range("bleedVal").Value = ""
' End checkbox manipulation
' Perform the same for the imposition button
ActiveSheet.Shapes("imposeCalc").OnAction = ""
ActiveSheet.Shapes("imposeCalc").Visible = False
' End interface manipulations
Range("forms").Select


it is better to put it in a separate procedure and call that procedure from each reapeating point. In this case there is a better way, but that is a good rule of thumb.

Because your code is writing to contiguous cells in each case, we can use that fact to have just one block of code that uses the index of the selection (instead of testing it as you do).

Like this



Dim SelectionIndex As Long

SelectionIndex = Worksheets("Sheet1").Range("szIND").Value
Worksheets("ref").Range("sizeDesc").Value = Worksheets("sizeRef").Range("A2").Offset(SelectionIndex - 1, 0)
Worksheets("Sheet1").Range("Width").Value = Worksheets("sizeRef").Range("B2").Offset(SelectionIndex - 1, 0)
Worksheets("Sheet1").Range("Height").Value = Worksheets("sizeRef").Range("C2").Offset(SelectionIndex - 1, 0)
Worksheets("Sheet1").Range("qUP").Value = Worksheets("sizeRef").Range("D2").Offset(SelectionIndex - 1, 0)
Worksheets("ref").Range("cutsPer").Value = Worksheets("sizeRef").Range("E2").Offset(SelectionIndex - 1, 0)
' Remove macro from bleed check box and enter null val for the bleedVal cell
' that is used in the quote and hide the check box
ActiveSheet.Shapes("Check Box 47").OnAction = ""
ActiveSheet.Shapes("Check Box 47").Visible = False
Worksheets("Sheet1").Range("bleedVal").Value = ""
' End checkbox manipulation
' Perform the same for the imposition button
ActiveSheet.Shapes("imposeCalc").OnAction = ""
ActiveSheet.Shapes("imposeCalc").Visible = False
' End interface manipulations
Range("forms").Select


Note that I store that index in a variable and use that varibale from thereon, much more efficient. When I use it, I use it to offset from the initial cells, A2, B2, etc.. As I am offsetting, I have to take that index -1.

We can make one more change to make it more efficient and more readable. We can set an implicit binding to a worksheet using With ... End With, and eliminate repeated statements and also be more efficient. We could pick Sheet1 or sizeRef, I picked the latter, but to be even more efficient I am going to explicitly bind to Sheet1 by setting a worksheet variable to that sheet and use that variable throughout, just as I did with the selection index.



Sub SheetSize()
Dim SelectionIndex As Long
Dim sh As Worksheet

Set sh = Worksheets("Sheet1")
SelectionIndex = sh.Range("szIND").Value
With Worksheets("sizeRef")

Worksheets("ref").Range("sizeDesc").Value = .Range("A2").Offset(SelectionIndex - 1, 0)
sh.Range("Width").Value = .Range("B2").Offset(SelectionIndex - 1, 0)
sh.Range("Height").Value = .Range("C2").Offset(SelectionIndex - 1, 0)
sh.Range("qUP").Value = .Range("D2").Offset(SelectionIndex - 1, 0)
sh.Range("cutsPer").Value = .Range("E2").Offset(SelectionIndex - 1, 0)
End With

' Remove macro from bleed check box and enter null val for the bleedVal cell
' that is used in the quote and hide the check box
ActiveSheet.Shapes("Check Box 47").OnAction = ""
ActiveSheet.Shapes("Check Box 47").Visible = False
sh.Range("bleedVal").Value = ""
' End checkbox manipulation
' Perform the same for the imposition button
ActiveSheet.Shapes("imposeCalc").OnAction = ""
ActiveSheet.Shapes("imposeCalc").Visible = False
' End interface manipulations
Range("forms").Select
End Sub

Papadopoulos
04-11-2010, 08:19 AM
Thanks,
This should take me a long way in improving my project!

Bob Phillips
04-11-2010, 09:03 AM
Actually, we can go one further step



Sub SheetSize()
Dim SelectionIndex As Long
Dim sh As Worksheet

Set sh = Worksheets("Sheet1")
SelectionIndex = sh.Range("szIND").Value
With Worksheets("sizeRef")

Worksheets("ref").Range("sizeDesc").Value = .Range("A2").Offset(SelectionIndex - 1, 0)
sh.Range("Width").Value = .Range("B2").Offset(SelectionIndex - 1, 0)
sh.Range("Height").Value = .Range("C2").Offset(SelectionIndex - 1, 0)
sh.Range("qUP").Value = .Range("D2").Offset(SelectionIndex - 1, 0)
sh.Range("cutsPer").Value = .Range("E2").Offset(SelectionIndex - 1, 0)
End With

' Remove macro from bleed check box and enter null val for the bleedVal cell
' that is used in the quote and hide the check box
With ActiveSheet

With .Shapes("Check Box 47")

.OnAction = ""
.Visible = False
End With

sh.Range("bleedVal").Value = ""
' End checkbox manipulation
' Perform the same for the imposition button
With .Shapes("imposeCalc")

.OnAction = ""
.Visible = False
End With
End With

' End interface manipulations
Range("forms").Select
End Sub