How to break up procedures

jmriddic

Registered User.
Local time
Today, 23:44
Joined
Sep 18, 2001
Messages
150
I have been doing some Access to Word automation coding so when they click on a button the document is populated. Unfortuantely now it says that my procedure is too big which is news to me since I never saw the error before. It suggested I break it into smaller procedures. The procedure and another one are quite large because of code addition Probably breaking it in half would work but I am unsure how to specify the procedure correctly so the code flow will stay the same and not error out. Any suggestions?
 
Break your code up into smaller functions.

Make sure each function is performing just one part of the entire code. Make each function something that just does one thing or a chuck of code that can be understood on its own (can stand alone).
e.g. if I have a form where people enter dimensions for a circle and a rectangle and want the area or circumference/perimeter of each shape. I would have four functions. Two for area and two for circumference/ perimeter.

Then call the functions from the main code.

This will actually help you debug each function rather than going through many wndows of code.
 
I don't think I've ever seen that one before. First things first. Count the number of lines of code in the module. Now use the Help menu to look up either Limits or Specifications. (Which one you need depends on which version of Access you are using. One of them will be right.)

Find the limits/specifications for modules. That will tell you the size of the biggest allowed module. Then you can decide if the module you've got is too big, and by how much.

BTW, if in doubt, you can export your module, then read it back into Word and have Word do line/word counts for you.

Now, now to split a module? Egad, you've asked a devilishly hard question, my friend. There are guidelines to be followed, to be sure, but it is hard to say what you need to do for your specific situation. I can only answer in generalities here.

1. If you see the same sort of thing happening over and over again in various parts of the same module only with different variable names, you might have just found a candidate for conversion to a function or procedure - and your different variable names become formal parameters for the function calls you need. Replace the "same thing happening over and over" with a single-line function/subroutine call.

2. If you are following one theme of action in your module and you leave that theme behind for another in the same module, you have found a split point. Make BOTH PARTS into subroutines that will then be called from a higher level master procedure. I.e. processing file A with one set of input rules, then processing file B with a different set of input rules. Merits having two input routines.

3. Where you see repetition in loops, even when the loops are themselves not similar enough to be covered under my item 1, it is SOMETIMES a good idea to make a subroutine out of a loop that has a relatively tight locus of code around it. For instance, if you have a loop that scans a recordset for something, maybe you can make the scanner into a subroutine even if you only use it in one place - because the scanner will probably have very different error requirements from the code around it. And there is another key to routine splitting...

4. When your error requirements change, you might find that the boundary is a good place to break a routine apart. I.e. up to point A in the code, you only trap one class of errors. But after point A until point B, you have to trap two classes of errors. Isolating the code makes the trap routines easier to write because they don't have to be so very generalized.

5. The rule of thumb USED TO BE that if you had more than about two pages of printed code, it was time to break it up into parts. These days, that might be a little small - but it is not FAR from right.

6. Try to read your code in a straight sitting. When you find yourself starting to nod off, you've passed a split point.
 
Code Attached

Hi Doc man,

I am going to attach the code for the module as a txt file. Maybe you can offer some suggestions that way . I do have a lot of reptition that is for sure but I didn't know really how to cropped alot of the code off. I have a table in my word document that has cells and rows and subcells and subrows and alot of my code is populating the cells based on bookmarks and deleting the rows and subrows if the bookmarks are not populated. FYI for the code is that competency info is in the first column of the document and the assessment is in the second coulmn which may have mutiple instances so I defined 12 assessment subcells and subrows(there are two columns following) for each competency so that explains why they are processed first.

Also Didn't see limits on lines it but I remember something about size being less than 64k. I am using Access 2000.
 

Attachments

I cannot grab your code and look at it because I work at a U.S. military installation. Our firewall eats attachments like candy. Our security officer files his teeth, just waiting to bite us on the butt if we try to break the rules on external file transfers to/from the base. I'm afraid I cannot be more hands-on than I already have been. Sorry.
 
???

Well,

the attachment is VBA but its in a text file. How else am I suppose to attach it?
 
Code for one instance

Doc man or whoever:

Here is a chunk of the code for one instance of the assessment subcells and what is taking up the chunk of the code. I have 25 instances of these and they only differ in the name of the assessment and the name of the bookmarks:
.ActiveDocument.Bookmarks("Assessment1").Select
If IsNull(Forms!HumanResourcesForm3!Assessment1) Then
.Selection.Text = ""
Else
MyString = (CStr(Forms!HumanResourcesForm3!Assessment1))
MyArray = Split(MyString, vbCrLf, -1, 1)
ArraySize = UBound(MyArray)
For Par = 0 To ArraySize
Select Case Par
Case 0
.Selection.Text = MyArray(0)
Case 1
.ActiveDocument.Bookmarks("Assessment1b").Select
.Selection.Text = MyArray(1)
Case 2
.ActiveDocument.Bookmarks("Assessment1c").Select
.Selection.Text = MyArray(2)
Case 3
.ActiveDocument.Bookmarks("Assessment1d").Select
.Selection.Text = MyArray(3)
Case 4
.ActiveDocument.Bookmarks("Assessment1e").Select
.Selection.Text = MyArray(4)
Case 5
.ActiveDocument.Bookmarks("Assessment1f").Select
.Selection.Text = MyArray(5)
Case 6
.ActiveDocument.Bookmarks("Assessment1g").Select
.Selection.Text = MyArray(6)
Case 7
.ActiveDocument.Bookmarks("Assessment1h").Select
.Selection.Text = MyArray(7)
Case 8
.ActiveDocument.Bookmarks("Assessment1i").Select
.Selection.Text = MyArray(8)
Case 9
.ActiveDocument.Bookmarks("Assessment1j").Select
.Selection.Text = MyArray(9)
Case 10
.ActiveDocument.Bookmarks("Assessment1k").Select
.Selection.Text = MyArray(10)
Case 11
.ActiveDocument.Bookmarks("Assessment1l").Select
.Selection.Text = MyArray(11)
Case Else
Stop
End Select
Next Par
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1b") = True Then
.ActiveDocument.Bookmarks("Assessment1b").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1c") = True Then
.ActiveDocument.Bookmarks("Assessment1c").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1d") = True Then
.ActiveDocument.Bookmarks("Assessment1d").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1e") = True Then
.ActiveDocument.Bookmarks("Assessment1e").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1f") = True Then
.ActiveDocument.Bookmarks("Assessment1f").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1g") = True Then
.ActiveDocument.Bookmarks("Assessment1g").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1h") = True Then
.ActiveDocument.Bookmarks("Assessment1h").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1i") = True Then
.ActiveDocument.Bookmarks("Assessment1i").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1j") = True Then
.ActiveDocument.Bookmarks("Assessment1j").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1k") = True Then
.ActiveDocument.Bookmarks("Assessment1k").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1l") = True Then
.ActiveDocument.Bookmarks("Assessment1l").Select
.Selection.Rows.Delete
End If

Keep in mind I need to keep the word document open if was to break it up and not loose any functionality. i would not be opposed to doing functions since I have another procedure which might exceed the size and could reference the same named assessment fields in Access as well as the same named bookmarks in Word. I just don't know how to go about in this instance.
 
Code:
For Par = 0 To ArraySize
Select Case Par
Case 0
.Selection.Text = MyArray(0)
Case 1
.ActiveDocument.Bookmarks("Assessment1b").Select
.Selection.Text = MyArray(1)
Case 2
.ActiveDocument.Bookmarks("Assessment1c").Select
.Selection.Text = MyArray(2)
Case 3
.ActiveDocument.Bookmarks("Assessment1d").Select
.Selection.Text = MyArray(3)
Case 4
.ActiveDocument.Bookmarks("Assessment1e").Select
.Selection.Text = MyArray(4)
...
If .ActiveDocument.Bookmarks.Exists("Assessment1b") = True Then
.ActiveDocument.Bookmarks("Assessment1b").Select
.Selection.Rows.Delete
End If
If .ActiveDocument.Bookmarks.Exists("Assessment1c") = True Then
.ActiveDocument.Bookmarks("Assessment1c").Select
.Selection.Rows.Delete
End If
...

Try something a bit less ugly and bulky. Build an array with your bookmark names that match your case names in some orderly way. OR have a table that links your Par variable to the name via a lookup if an array is too big.

Code:
For Par = 0 to Arraysize
if Par > 0 then
.ActiveDocument.Bookmarks(bkmkname(par)).select
end if
.Selection.Text = MyArray(Par)
...
If .ActiveDocument.Bookmarks.Exists(bkmkname(par)) = True Then
.ActiveDocument.Bookmarks(bkmkname(par)).Select
.Selection.Rows.Delete
End If

You don't need the big hompin' case statements if the name of the bookmark is the only variant. Just use an array to hold the names that you need. Or a recordset that you can step through to get the potential bookmark names.
 
Could you explain using my variables?

Hi Doc man,

Thanks for the reply. Could you explain using the variables from my code? I grasp some of what you said just not all. Another thing is that zero positionin the array is populated too. Thanks.
 
Docman,

I am not getting what you are saying:

if Par > 0 then
.ActiveDocument.Bookmarks(bkmkname(par)).select

The if statement wouldn't work since there is data in the Zero position in the array. Also you said something about building an array using your bookmark names that match your case names. The code above doesn't clarify that for me. I have the following bookmarks: Assessment1,Assessment1b-Assessment1l. Not seeing the connection using any of those in place of bkmkname.
 
Sorry not to get back faster, been busy. My boss thinks I should actually do some work now and then. The affrontery of it all! :rolleyes:

What I was suggesting is this... You were using the case statement to, in effect, just supply a number and a text string. You had one case for each number. But other than the number and the text string, every case was pretty much the same code. You were supplying the collection("name") syntax for each case, but you could equally look up the name from an array or recordset or something else and use the collection(name-holder-variable) syntax in a non-loop structure - or at least a loop with different structure.

In other words, you had a number and that number appeared to be suitable as a selector for the name as well. So why store pot-loads of constants in code (which is what CASE + COLLECTION("NAME") syntax does) when you can find another way to store your Par value and required bookmark names. Or perhaps generate them, for all I can tell. See, Access doesn't give a rat's patootie about what goes inside the COLLECTION() parentheses as long as it is either a name or a number. You don't need all the separate constants. Variables work in that context excellently.

Suppose you had a recordset that contained records with a number and a name you use for bookmarks. Your loop could step through the records of the recordset, pulling in the number and name with a For Each loop or with a loop that tests for EOF on the recordset. NO constants involved except the EOF marker test.

Now, there is the issue that this could be a long document you are scanning paragraph by paragraph. This is why I suggested a loop through an array if the number of elements isn't too large for your physical memory on your 'puter. At least this would be faster than resetting the recordset for each paragraph.
 
Followup

I agree that the structure could be better. I guess the first thing in my head looking at the snippet of code I had a question about is one do I have to specify the same mentioned bookmarks over and over as before. I don't think so but I am not sure. Two, I don't see anywhere where it says that it knows which array number knows which bookmark to populate. Before I knew array(0) went to Assessment1, Array(11) went to Assessment1l. And when I go to the second and third set and upward to set 25 how do I know each array entry will go to the right bookmarks since the array will repopulate after each set I believe . Maybe in that aspect I am not understanding your code. Other things I have commented on below using your code to illustrate

For Par = 0 to Arraysize
if Par > 0 then 'cannot be used since 0 position is used.
.ActiveDocument.Bookmarks(bkmkname(par)).select
' how does know which bookmark to select and how is it stored in an array?
end if
.Selection.Text = MyArray(Par)
...
If .ActiveDocument.Bookmarks.Exists(bkmkname(par)) = True Then
.ActiveDocument.Bookmarks(bkmkname(par)).Select
.Selection.Rows.Delete
End If

If you could illustrate the bookmarks I have in my code it might make more sense to me.
 
Code:
For Par = 0 To ArraySize
Select Case Par
Case 0
.Selection.Text = MyArray(0)
Case 1
.ActiveDocument.Bookmarks("Assessment1b").Select
.Selection.Text = MyArray(1)
Case 2
.ActiveDocument.Bookmarks("Assessment1c").Select
.Selection.Text = MyArray(2)
Case 3
.ActiveDocument.Bookmarks("Assessment1d").Select
.Selection.Text = MyArray(3)

In this code, the only difference between case 1 and case 2 occurs in MyArray(n) (n=1 or 2, which matches Par) and "Assessment1b" vs. "Assessment1c". I can imagine building a recordset ahead of time where the records are

0, "assessment1a"
1, "assessment1b"
2, "assessment1c"
and then use a DLookup to take Par and find the string. Or pre-read an array of assessment names.

Now, I'm not sure where you were going with this part...

Code:
MyString = (CStr(Forms!HumanResourcesForm3!Assessment1))
MyArray = Split(MyString, vbCrLf, -1, 1)
ArraySize = UBound(MyArray)

But if you are telling me that the issue is that I need to take the string "Assessment" and append something unique to it, I can GENERATE that rather than enumerate the elements in fixed code. I.e.

Code:
Assessname = "Assessement1" & chr$( par + Asc("a"))

Then the only question is how you decide it is Assessment1 vs. Assessment2 etc. etc.

My point was that if you have a size problem, don't repeat constant-laden code. Generate or store the parts that are different and tighten the loop considerably that way.
 
Maybe I am confusing you.

Hi Doc Man,

I see what you are saying more clearly. Right the only difference is the numbering of the array entry and I see the validity of either option. Problem is using the recordset option is after the first set of code from above which has the bookmarks Assessment1-Assessment1l(no Assessment1a) the numbers at least in my code would start over. So bookmark Assessment2 would have 0, Assessment2b would have 1 and so forth. Remember I have 25 sets of bookmarks numbered as mentioned. So I don't know how that would work. I would think the array idea would work if for example if I after creating the array, which I would recreate for each set, Entry(0) knew how to go for example to Assessment 1, Entry(1) to Assessment1b etc. and when we got to the second set Entry(0) went to Assessment2 ,Entry(1) to Assessment2b and so on. I guess adding to the problem and what I failed to mentioned that there are a couple of other bookmarks named Job Title and Department at the beginning. Don't know if that adds to the problem. Referring to the code you don't understand that code is taking what is in each field on the form and putting it in a variable. I take that variable and split the string up and that is how I get my array. I then get the number of entries and case on that.
I just wish Access didn't have a limit of the amount of code because I knew it worked. I appreciate your help.
 
My suggestion still helps a little, because even if you have to start with different bookmarks and have to be a little awkward in defining the base string (Assessment1, Assessment2, etc.), you can still generate the remainder of the string you want rather than having to store it in bulky constants and a complex case statement. I'll tell you right now, you could probably cut that code by huge chunks just by replacing the messy case statements that supplied PAR and the bookmark text. I can't state it enough. Don't repeat literals when you can generate it in the first place. Don't use CASE like you did inside your loop when you can generate the parts you need.
 
I understand

Doc Man,

I understand the Case statement is the major hindrance in the whole code. I would be willing to redo the bookmarks at this point. I am just totally frustrated with the whole thing because if I knew the case statement was going to cause this much problems I would have tried something else. Based on what we have discovered and discussed could you give me an example of what might work here as far as the code goes and what do you suggest I rename the bookmarks. I will go from there. Thanks.
 

Users who are viewing this thread

Back
Top Bottom