When should code be broken down into proceedures

[Aussiebear]

Its an arbitrary thing i guess, but when should code be broken down into sub modules? Listed below is a section of code that had been presented to this forum back in 2009. Not only is it very busy with its intentions, but from a coders point of view would you attempt something like this?



Private Sub CommandButton1_Click()
Dim i, j As Integer
Dim rng As Range
Dim ws As Worksheet
Dim r1 As Integer, c1 As Integer
Dim r2 As Integer, c2 As Integer
UserForm1.Show
ReDim A(1 To NumberOfCampaigns, 1 To NumberOfQualities) As Variant
ReDim ATRANSPOSE(1 To NumberOfQualities, 1 To NumberOfCampaigns) As Variant
ReDim ATRANSPOSEA(1 To NumberOfQualities, 1 To NumberOfQualities) As Variant
ReDim ATRANSPOSEB(1 To NumberOfQualities, 1 To 1) As Variant
ReDim XATRANSPOSEB(1 To 1) As Variant
ReDim B(1 To NumberOfCampaigns, 1 To 1) As Variant
ReDim BTRANSPOSE(1 To 1, 1 To NumberOfCampaigns) As Variant
ReDim X(1 To 1, 1 To NumberOfQualities) As Variant
If Arng Is Nothing Then Exit Sub
Set ws = ActiveSheet
ws.Activate
'Get used range
Set rng = ws.UsedRange
r1 = rng.Row
r2 = rng.Rows.Count + r1 - 1
c1 = rng.Column
c2 = rng.Columns.Count + c1 - 1
'Print A matrix below used range
ws.Cells(r2 + 2, c1).Value = "A MATRIX"
ws.Cells(r2 + 2, c1).Font.Bold = True
ws.Cells(r2 + 2, c1 + NumberOfQualities + 2).Value = "A TRANSPOSE MATRIX"
ws.Cells(r2 + 2, c1 + NumberOfQualities + 2).Font.Bold = True
Set rng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfCampaigns - 1, c1 + NumberOfQualities - 1))
Arng.Copy rng
rng.BorderAround LineStyle:=xlContinuous, Weight:=xlThin
'Initialize array A with values from range Arng
For i = 1 To NumberOfCampaigns
For j = 1 To NumberOfQualities
A(i, j) = Arng(i, j)
Next j
Next i
ATRANSPOSE = WorksheetFunction.Transpose(A)
r1 = r2 + 3
c1 = c1 + NumberOfQualities + 1
r2 = r2 + 3 + NumberOfQualities - 1
c2 = c1 + NumberOfCampaigns - 1
Set ATRANSPOSErng = Range(Cells(r1, c1), Cells(r2, c2))
For i = 1 To NumberOfQualities
For j = 1 To NumberOfCampaigns
ATRANSPOSErng(i, j) = ATRANSPOSE(i, j)
Next j
Next i
ATRANSPOSErng.Select
Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin
'Get used range
Set rng = ws.UsedRange
'Print B next to A TRANSPOSE
ws.Cells(r1 - 1, c2 + 2).Value = "b MATRIX"
ws.Cells(r1 - 1, c2 + 2).Font.Bold = True
For i = 1 To NumberOfCampaigns
ActiveSheet.Cells(r1 + i - 1, c2 + 2).Value = 1
B(i, 1) = 1#
Next i
BTRANSPOSE = WorksheetFunction.Transpose(B)
Set Brng = Range(Cells(r1, c2 + 2), Cells(r1 + NumberOfCampaigns - 1, c2 + 2))
Brng.BorderAround LineStyle:=xlContinuous, Weight:=xlThin
'Print ATRANA next to ATRAN
Set rng = ws.UsedRange
r1 = rng.Row
r2 = rng.Rows.Count + r1 - 1
c1 = rng.Column
c2 = rng.Columns.Count + c1 - 1
ws.Cells(r2 + 2, c1).Value = "PRODUCT OF A TRANSPOSE AND A"
ws.Cells(r2 + 2, c1).Font.Bold = True
ATRANSPOSEA = WorksheetFunction.MMult(ATRANSPOSE, A)
Set ATRANSPOSEArng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + NumberOfQualities - 1))
For i = 1 To NumberOfQualities
For j = 1 To NumberOfQualities
ATRANSPOSEArng(i, j) = ATRANSPOSEA(i, j)
Next j
Next i
ATRANSPOSEArng.Select
Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin
' Print ATRANSPOSE b next to ATRANSPOSEA
ws.Cells(r2 + 2, c1 + NumberOfQualities + 1).Value = "PRODUCT OF A TRANSPOSE AND b"
ws.Cells(r2 + 2, c1 + NumberOfQualities + 1).Font.Bold = True
ATRANSPOSEB = WorksheetFunction.MMult(ATRANSPOSE, B)
Set ATRANSPOSEBrng = Range(Cells(r2 + 3, c1 + NumberOfQualities + 1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + NumberOfQualities + 1))
For i = 1 To NumberOfQualities
ATRANSPOSEBrng(i) = ATRANSPOSEB(i, 1)
Next i
ATRANSPOSEBrng.Select
Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin
Set rng = ws.UsedRange
r1 = rng.Row
r2 = rng.Rows.Count + r1 - 1
c1 = rng.Column
c2 = rng.Columns.Count + c1 - 1
ws.Cells(r2 + 2, c1).Value = "CONSUMPTION OF REFRACTORY MATERIAL PER TONNE OF EACH QUALITY"
ws.Cells(r2 + 2, c1).Font.Bold = True
Set Xrng = Range(Cells(r2 + 3, c1), Cells(r2 + 3, c1 + NumberOfQualities - 1))
Xrng.Select
Xrng.BorderAround LineStyle:=xlContinuous, Weight:=xlThick, Color:=vbRed
' Initialize array X to zeros
For i = 1 To NumberOfQualities
Xrng(1, i) = i / NumberOfQualities
X(1, i) = Xrng(1, i)
Next i
ws.Cells(r2 + 2, c1 + NumberOfQualities + 1).Value = "Objective Function"
ws.Cells(r2 + 2, c1 + NumberOfQualities + 1).Font.Bold = True
' Initialize objective function cell with formula intact.:banghead:
' The FormulaArray requires R1C1 notation. There is a range.Address
' property that returns the R1C1 coordinates for the particular range in quotes.
ws.Cells(r2 + 3, c1 + NumberOfQualities + 1).Select
Selection.FormulaArray = WorksheetFunction.MMult(Xrng, Application.WorksheetFunction.MMult(ATRANSPOSEArng, Application.WorksheetFunction.Transpose(Xrng)))
Selection.BorderAround LineStyle:=xlDouble
ws.Cells(r2 + 4, c1 + NumberOfQualities + 1).Select
Selection.FormulaArray = WorksheetFunction.MMult(Xrng, ATRANSPOSEBrng)
Selection.BorderAround LineStyle:=xlDouble
Set rng = ws.UsedRange
r1 = rng.Row
r2 = rng.Rows.Count + r1 - 1
c1 = rng.Column
c2 = rng.Columns.Count + c1 - 1
ws.Cells(r2 + 2, c1).Value = "X CONSTRAINED TO BE POSITIVE"
ws.Cells(r2 + 2, c1).Font.Bold = True
Set Constraintrng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + 2))
Constraintrng.Select
For i = 1 To NumberOfQualities
Constraintrng(i, 1).Formula = Xrng(1, i)
Constraintrng(i, 2) = ">="
Constraintrng(i, 3) = 0.001
Next i
Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin, Color:=vbBlue
Exit Sub
' Calls to the Solver family of functions with the correct arguments.
' A path reference to the add-in location is available for these calls.
' MaxMinVal in SolverOk(SetCell, MaxMinVal, ValueOf, ByChange) calls can take on the following values
' 1 is maximize
' 2 is minimize
' 3 is match a value
' Relation in SolverAdd(cellref, Relation, Formulatext) calls can taken on the following values
' 1 is <=
' 2 is =
' 3 is >=
' 4 is integer valued
' 5 is binary valued
SolverOk SetCell:=Selection.Address, MaxMinVal:=2, ValueOf:="0", ByChange:=Xrng.Address
For i = 1 To NumberOfQualities
SolverAdd CellRef:=Constraintrng(i, 1).Address, Relation:=3, FormulaText:=Constraintrng(i, 3).Address
Next i
SolverOptions MaxTime:=100, Iterations:=100, Precision:=0.000001, AssumeLinear _
:=False, StepThru:=False, Estimates:=1, Derivatives:=1, SearchOption:=1, _
IntTolerance:=5, Scaling:=False, Convergence:=0.0001, AssumeNonNeg:=True
SolverSolve
End Sub


How should this be handled?

[Logit]

My $.02 cents ....

Personal preference ? And of course there is the 64k limit on each module.

My view ... if the code looks 'confusing' or 'busy' ... and there is a means to break it down into smaller sections ... that might be an option for me.

Your thoughts ?

[Aussiebear]

Hmmm... now there a logical solution. However, just to be the devils advocate.. If you were presented with this code what would you recomend.

[jdelano]

I tend to rely heavily on DRY and separation of concerns. The latter needing more time to completely understand what this wants to do. A cursory pass I'd do something like:



Dim rng As Range
Dim r1 As Integer, c1 As Integer
Dim ws As Worksheet
Dim r2 As Integer, c2 As Integer
Dim i, j As Integer


Private Sub GetUsedRange()


'Get used range
Set rng = ws.UsedRange
r1 = rng.Row
r2 = rng.Rows.Count + r1 - 1
c1 = rng.Column
c2 = rng.Columns.Count + c1 - 1


End Sub


Private Sub SetCellValue(rw As Long, col As Integer, cellValue As String)


ws.Cells(rw, col).Value = cellValue
ws.Cells(rw, col).Font.Bold = True


End Sub


Private Sub InitializeArray(ByRef leftArray As Variant, leftLoopTo As Integer, rightArray As Variant, rightLoopTo As Integer)


'Initialize array A with values from range Arng
For i = 1 To leftLoopTo
For j = 1 To rightLoopTo
leftArray(i, j) = rightArray(i, j)
Next j
Next i


End Sub




Private Sub LargeSubroutine()

UserForm1.Show

ReDim A(1 To NumberOfCampaigns, 1 To NumberOfQualities) As Variant
ReDim ATRANSPOSE(1 To NumberOfQualities, 1 To NumberOfCampaigns) As Variant
ReDim ATRANSPOSEA(1 To NumberOfQualities, 1 To NumberOfQualities) As Variant
ReDim ATRANSPOSEB(1 To NumberOfQualities, 1 To 1) As Variant
ReDim XATRANSPOSEB(1 To 1) As Variant
ReDim B(1 To NumberOfCampaigns, 1 To 1) As Variant
ReDim BTRANSPOSE(1 To 1, 1 To NumberOfCampaigns) As Variant
ReDim X(1 To 1, 1 To NumberOfQualities) As Variant

If Arng Is Nothing Then Exit Sub

Set ws = ActiveSheet
ws.Activate

GetUsedRange

'Print A matrix below used range
SetCellValue r2 + 2, c1, "A MATRIX"
SetCellValue r2 + 2, c1 + NumberOfQualities + 2, "A TRANSPOSE MATRIX"

Set rng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfCampaigns - 1, c1 + NumberOfQualities - 1))
Arng.Copy rng
rng.BorderAround LineStyle:=xlContinuous, Weight:=xlThin

'Initialize array A with values from range Arng
InitializeArray A, NumberOfCampaigns, Arng, NumberOfQualities

ATRANSPOSE = WorksheetFunction.Transpose(A)

r1 = r2 + 3
c1 = c1 + NumberOfQualities + 1
r2 = r2 + 3 + NumberOfQualities - 1
c2 = c1 + NumberOfCampaigns - 1

Set ATRANSPOSErng = Range(Cells(r1, c1), Cells(r2, c2))

InitializeArray ATRANSPOSErng, NumberOfQualities, ATRANSPOSE, NumberOfCampaigns

ATRANSPOSErng.Select
Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin

'Get used range
Set rng = ws.UsedRange

'Print B next to A TRANSPOSE
SetCellValue r1 - 1, c2 + 2, "b MATRIX"

For i = 1 To NumberOfCampaigns
ActiveSheet.Cells(r1 + i - 1, c2 + 2).Value = 1
B(i, 1) = 1#
Next i

BTRANSPOSE = WorksheetFunction.Transpose(B)

Set Brng = Range(Cells(r1, c2 + 2), Cells(r1 + NumberOfCampaigns - 1, c2 + 2))
Brng.BorderAround LineStyle:=xlContinuous, Weight:=xlThin

'Print ATRANA next to ATRAN
GetUsedRange

SetCellValue r2 + 2, c1, "PRODUCT OF A TRANSPOSE AND A"

ATRANSPOSEA = WorksheetFunction.MMult(ATRANSPOSE, A)

Set ATRANSPOSEArng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + NumberOfQualities - 1))

InitializeArray ATRANSPOSEArng, NumberOfQualities, ATRANSPOSEA, NumberOfQualities

ATRANSPOSEArng.Select

Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin

' Print ATRANSPOSE b next to ATRANSPOSEA
SetCellValue r2 + 2, c1 + NumberOfQualities + 1, "PRODUCT OF A TRANSPOSE AND b"

ATRANSPOSEB = WorksheetFunction.MMult(ATRANSPOSE, B)

Set ATRANSPOSEBrng = Range(Cells(r2 + 3, c1 + NumberOfQualities + 1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + NumberOfQualities + 1))

For i = 1 To NumberOfQualities
ATRANSPOSEBrng(i) = ATRANSPOSEB(i, 1)
Next i

ATRANSPOSEBrng.Select

Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin

GetUsedRange

SetCellValue r2 + 2, c1, "CONSUMPTION OF REFRACTORY MATERIAL PER TONNE OF EACH QUALITY"

Set Xrng = Range(Cells(r2 + 3, c1), Cells(r2 + 3, c1 + NumberOfQualities - 1))

Xrng.Select
Xrng.BorderAround LineStyle:=xlContinuous, Weight:=xlThick, Color:=vbRed

' Initialize array X to zeros
For i = 1 To NumberOfQualities
Xrng(1, i) = i / NumberOfQualities
X(1, i) = Xrng(1, i)
Next i

SetCellValue "Objective Function"

' Initialize objective function cell with formula intact.:banghead:
' The FormulaArray requires R1C1 notation. There is a range.Address
' property that returns the R1C1 coordinates for the particular range in quotes.
ws.Cells(r2 + 3, c1 + NumberOfQualities + 1).Select

Selection.FormulaArray = WorksheetFunction.MMult(Xrng, Application.WorksheetFunction.MMult(ATRANSPOSEArng, Application.WorksheetFunction.Transpose(Xrng)))
Selection.BorderAround LineStyle:=xlDouble

ws.Cells(r2 + 4, c1 + NumberOfQualities + 1).Select

Selection.FormulaArray = WorksheetFunction.MMult(Xrng, ATRANSPOSEBrng)
Selection.BorderAround LineStyle:=xlDouble

GetUsedRange

SetCellValue r2 + 2, c1, "X CONSTRAINED TO BE POSITIVE"

Set Constraintrng = Range(Cells(r2 + 3, c1), Cells(r2 + 3 + NumberOfQualities - 1, c1 + 2))
Constraintrng.Select

For i = 1 To NumberOfQualities
Constraintrng(i, 1).Formula = Xrng(1, i)
Constraintrng(i, 2) = ">="
Constraintrng(i, 3) = 0.001
Next i

Selection.BorderAround LineStyle:=xlContinuous, Weight:=xlThin, Color:=vbBlue

Exit Sub


' Calls to the Solver family of functions with the correct arguments.
' A path reference to the add-in location is available for these calls.
' MaxMinVal in SolverOk(SetCell, MaxMinVal, ValueOf, ByChange) calls can take on the following values
' 1 is maximize
' 2 is minimize
' 3 is match a value
' Relation in SolverAdd(cellref, Relation, Formulatext) calls can taken on the following values
' 1 is <=
' 2 is =
' 3 is >=
' 4 is integer valued
' 5 is binary valued

SolverOk SetCell:=Selection.Address, MaxMinVal:=2, ValueOf:="0", ByChange:=Xrng.Address

For i = 1 To NumberOfQualities
SolverAdd CellRef:=Constraintrng(i, 1).Address, Relation:=3, FormulaText:=Constraintrng(i, 3).Address
Next i

SolverOptions MaxTime:=100, Iterations:=100, Precision:=0.000001, AssumeLinear _
:=False, StepThru:=False, Estimates:=1, Derivatives:=1, SearchOption:=1, _
IntTolerance:=5, Scaling:=False, Convergence:=0.0001, AssumeNonNeg:=True
SolverSolve

End Sub





edit: forgot to update the initial sub with the loop to number

[Paul_Hossler]

My $0.02 -- and some personal style

It's hard to tell since some variables are not in scope (Arng, ws, etc.) and it's too much effort required to try and decypher what the code is doing :dunno

BTAIM,

I would also try to use self documenting variable names (even if more typing), for example



Dim dataRange As Range
Dim dataWorksheet As Worksheet
Dim dataRowStart As Integer, dataColumnStart As Integer
Dim dataRowEnd As Integer, dataColumnEnd As Integer



I'd have the click event handler just call call other subs in a standard module and each sub perform just one thing. Probably have to pass parameters.

UserForm ---


Option Explicit


Private Sub CommandButton1_Click()

If OkToContinue(Me.TextBox1.Value) Then
Call ShowValue(Me.TextBox1.Value)
Call AllDone

Else
MsgBox "Numeric, but >= 100"
End If


UserForm1.Hide
Unload UserForm1
End Sub







Standard Module--



Option Explicit


Sub driver()
Load UserForm1
UserForm1.Show
End Sub




Function OkToContinue(x As Variant) As Boolean


If IsNumeric(x) Then
If x < 100 Then
OkToContinue = True
Else
OkToContinue = False
End If

Else
OkToContinue = True
End If
End Function


Sub ShowValue(x As Variant)
MsgBox x
End Sub


Sub AllDone()
MsgBox "All Done"
End Sub

[Bob Phillips]

Your title asks when should code be broken down into modules, but the code you showed needed breaking down into separate procedures, as suggested by the others.

In my view, modules should be functionally oriented. I tend to have one for my version history, one for general globals, one for application specific globals, one for enums, one for types, one for messages, one for entry points, helper modules for supporting each entry point, and a bunch of utility modules (such as one that has a number of generic range functions, one for generic table functions, etc.). Plus classes and forms of course.

I personally do not worry about having too many modules, I would rather have 100 modules where one may only have one function in it rather than mix up functionality.

I also tend to go for lots of procedures in code modules. I remember Simon Murphy once saying at a conference that he believes no module should have more code than can be seen on a screen without scrolling. I buy into the principle of that, but I find it somewhat hard to stick to it when you add in error handling and/or debug trace code.

I tend to write my functionality all in one procedure to start with, get it working as best I can, and then I go through it and see which parts are doing some discrete action, and drop that into a separate private procedure. I find this is useful in that I concentrate on outcome to start with, but then by breaking it down I review it in a refactoring frame of mind.

I would also add a lot more spacing than your code example.

Works for me.

[Paul_Hossler]

I would also add a lot more spacing than your code example.

Works for me.


And your friend the{Tab} key to indent blocks of code

[Logit]

And your friend the{Tab} key to indent blocks of code

Using this approach would easily clean up the posted code ... making it more visually accepting. Good job.