0

I previously posted this in Code Review, and I guess it wasn't the right place. Hopefully this is.

I have created dozens of Subs for various actions for Excel worksheet order forms. Hundreds of these are done a day, and many things must be checked.

The routine is that when someone opens one of these worksheets, the first thing they do is run a macro that calls dozens of others. So it's essentially something like this:

Sub AllMacros()
Call Macro1
Call Macro2
Call Macro3
Call Macro4
Call Macro5
Call Macro6
Call Macro7
Call Macro8
Call Macro9
Call Macro10
Call Macro11
Call Macro12
Call Macro13
Call Macro14
Call Macro15
Call Macro16
Call Macro17
Call Macro18
Call Macro19
Call Macro20
End Sub

I did it this way because there are times when only one of those Subs needs to be run, and so they will be executed individually.

My main question is this: Is this technique inherently not a good idea? I understand I'm not showing all the code, but that's not the point. There's way too much code to post here. I suppose I'm looking for the consensus answer to be either "Yes, that's fine." or "No, it's better to do it this way."

Robby
  • 843
  • 3
  • 19
  • 53
  • 3
    Well, hopefully your procedures are not called `Macro1` to `Macro20`. Meaningful names are really good practice for variable and procedure/function names. Also it is a very good idea to split several different tasks into several procedures, so one procedure does only one thing. That way it is much easier to re-use code and prevent code duplication. So nothing wrong to have a procedure which call all the parts. – Pᴇʜ Jun 07 '18 at 13:32
  • They are not actually called Macro1-Macro20. They're called all sorts of various things, but thanks. – Robby Jun 07 '18 at 13:36

2 Answers2

13

If you posted your actual real code on CR, you'd get a much more helpful & meaningful answer.

From a design standpoint, what you've got is a "coordinator" whose role is to invoke other procedures in a specific order, which isn't bad in itself.

From a pragmatic point of view, what you've got is not any different than any other series of executable statements in any other procedure: any executable statement is going to be invoking a member of something at one point or another.

The difference between inlining your 20 macros into this "god procedure", and splitting them like you did, is abstraction - one of the key concepts of object-oriented programming.

Whether you have good abstractions is impossible to tell with such fake names and without knowing specifically what problem is being solved by all these macros, and that is exactly why Code Review wants to see your real code, and why this:

I suppose I'm looking for the consensus answer to be either "Yes, that's fine." or "No, it's better to do it this way."

Is unanswerable with hypothetical Macro1...Macro20 stub code.

If the interactions between the macros is such that global state is altered between them and the order of execution is critical, then what you've got is bad because you've got implicit temporal coupling: if the order of execution changes, things fall apart / break - much like when you Activate a cell and then work off ActiveCell or Selection, like macro-recorder code.

So if instead of this:

OpenFiles
ProcessNewData
GenerateReports
SaveCSVs
'...

You had this:

Set files = OpenFiles
Set reportData = ProcessNewData(files, targetBook)
Set reports = GenerateReports(reportData)
SaveCSVs(reports)

You would have made the temporal coupling much more explicit, removed the chance for executing one of the macros in the wrong order, and eliminated the global state.

Your answer is "it depends what your real code looks like".

Mathieu Guindon
  • 69,817
  • 8
  • 107
  • 235
  • 2
    Excellent comments.............they apply to software design in general, not just this specific example – Gary's Student Jun 07 '18 at 13:58
  • I originally thought Code Review was more appropriate since my code worked, but I wondered if there was a better way of doing it. I understand that it depends on what all the code is, but there's just entirely too much to post here. – Robby Jun 07 '18 at 16:54
  • @Robby in that case I'd post the "master macro" along with 2 or 3 of the "child" macros, including the declarations for whatever global state they're sharing. Code Review posts can be twice as large as SO posts (that's 64K characters), exactly because reviewing code can't work off a [mcve]. The problem was more with asking about "best practices in general" vs "does this code follow best practices" - I have an OSS project with over 200K lines of code, and I can still put up self-contained parts of it on CR =) – Mathieu Guindon Jun 07 '18 at 17:14
2

There is nothing "wrong" in using a master macro to call others, but consider:

Sub Suggestion()
    For i = 1 To 20
        Application.Run "Macro" & i
    Next i
End Sub

You can extend this idea by creating arrays of String variables. Each array being a sequence of macro calls.

EDIT#1:

An example with "real" macro names:

Sub NextSuggestion()
    arry = Array("OpenFiles", "ProcessNewData", "GenReports", "SaveCSVs")
    For Each a In arry
        Application.Run a
    Next a
End Sub
Gary's Student
  • 95,722
  • 10
  • 59
  • 99
  • My real macro names are not so generic as Macro1, 2, 3, etc. I just used those as placeholder names for the sake of my question. So I don't think this would work, would it? – Robby Jun 07 '18 at 13:36
  • @Robby Then you would use an array *(but still a loop)* Want an example ?? – Gary's Student Jun 07 '18 at 13:43
  • @Robby See my **EDIT#1** – Gary's Student Jun 07 '18 at 13:48
  • 1
    Although it looks pretty having a loop that loops through each macro, in reality, it makes no difference going line by line or looping, The code is still going the same distance. – Davesexcel Jun 07 '18 at 16:03
  • 1
    @Davesexcel With regard to programming, you are completely correct. From the user's perspective, several arrays of function calls can be constructed, each array for some specific purpose like creating monthly reports, or quarterly reports or making powerpoint presentations or sending out emails, etc. – Gary's Student Jun 07 '18 at 16:14