Some simple naming observations
Private Function VisioInstanceExist() As Boolean
On Error Resume Next
Dim visioApp As Object
Set visioApp = GetObject(, VISIO_CLASS_ID)
VisioInstanceExist = Not visioApp Is Nothing
End Function
Looks like a round-about way to creating an instance of Visio. It's not really a boolean at all. If you checked if it existed and returned a boolean to decide whether you need to create one, then it would be boolean.
Moreover, I don't see any way that the instance of Visio could exist at that point, it's the first thing you do and you've not set anything to the object.
I'd unrefactor this
Private Function GetVisioInstance() As Object
Dim visioApp As Object
On Error Resume Next
Set visioApp = GetObject(, VISIO_CLASS_ID)
If visioApp Is Nothing Then
GetVisioInstance = StartVisioInstance
Else
Set GetVisioInstance = visioApp()
End If
End Function
See update at bottom.
That's really not too bad of a function and it eliminates 3 others. Your call on the error checking.
Let's look at what procedures we have left (ignore that it's in excel, that shouldn't matter)
created in RubberDuck-VBA

Wait! There's no .Initialize()
function. Maybe you can do all the creation and getting during initialization?
Otherwise -
- ActivateVisioInstance
- ActivateVisioWindow
- DetermineDesiredVisioWindow
- LoadVisioDrawing
- MaximizeVisioWindow
- OpenVisioDrawing
- SyncVisioDrawing
- VisioScreenUpdating
Now, I'll admit I'm not a Visio expert, but isn't this being run in Visio? Or, if it isn't, this Class
has Visio in its name.
I think it's pretty clear we're dealing with Visio, don't you? That's just muddying the naming waters and really doesn't help to understand what is going on with each function. Why not just drop it?
- ActivateInstance
- ActivateWindow
- DetermineDesiredWindow
- LoadDrawing
- MaximizeWindow
- OpenDrawing
- SyncDrawing
- ScreenUpdating
Hm, that's better. I don't like that ScreenUpdating
function, there's already Application.ScreenUpdating
. Seems maybe that could just be a boolean property to Let
and Get
? I only actually see that property change once and then back.
Private Sub ActivateVisioInstance(ByRef app As Object)
AppActivate app.window.Caption
End Sub
Here you're using the AppActivate statement, but you've wrapped it in a ActivateInstance
method. And you're using a caption that is a property of Window
(the visio object, not your variable). But I don't see where you're using that property, I don't know what the caption will be. It seems like your ActivateInstance
procedure is less efficient and/or clear than if you just used the AppActivate
statement in the first place. If you're going to wrap it in something, make use of that! You aren't even taking a caption argument!
SyncVisioDrawing drawing, someData
Private Sub SyncVisioDrawing(ByRef drawing As Object, ByVal someData As String)
drawing.Title = someData
End Sub
You're passing an application object into a procedure to set its title, which you're also passing to the procedure? so it's SetDrawingTitle
? Using that procedure isn't needed. If it is, name it better, take a different argument, maybe turn it into a function to get the title, or even better - make it a property of the Class.
One thing you asked about was OpenVisioDrawing
being confused with LoadVisioDrawing
. Yes, that's not great naming, you're right. What is OpenVisioDrawing
doing?
- Checking and creating an instance of Visio
- Loading a drawing into Visio
- Fixing the view of Visio to what you need
That's a lot of things, and a lot of those things are broken out to other procedures already (which is great!). I'd say the sub is managing all of the visio procedures, or being the interface to visio (that's the class name right?).
Naming Variables
Dim app As Object
Dim drawing As Object
Dim window As Object
Dim visioApp As Object
Const VISIO_CLASS_ID As String = "Visio.Application"
Const visOpenRW = &H20
Const visWSMaximized = &H40000000
Your first constant is UPPER_SNAKEY_CASE but the other two aren't. They aren't given Types either, so they are variants. What should they be? Strings I must assume. So make them strings.
Also, what are those values? &H20
and &H40000000
- hex notations? Of what? ASCII characters? Those constants names should at least tell me what they should be! Otherwise
Const visOpenRW = &H20
Set LoadVisioDrawing = app.Documents.OpenEx(filename, visOpenRW)
I have no idea what that should do. I don't know offhand what .OpenEx
parameters are (because I'm no Visio expert). Let's find out.
What? Those are flags and it is an integer? And visOpenRW
already is the constant for it. Okay, now a lot more of that makes sense, but if there's already a constant, just use it as the flag, no need to tell me there's a constant, apparently an integer, that goes as an argument (flag) into an expression. I'd know more just by
Set LoadVisioDrawing = app.Documents.OpenEx(filename, visOpenRW)
At least I won't chase my tail around for your constant's hex notation only to find out the flag visOpenRW
is a built-in constant for that expression.
The variable app
is what I would call vague. I see you used visioApp
once and most of your functions said "visio" - why not just use visioApp
or visioInstance
or something similar as your variable name? That will make sure I know we're in Visio.
You've also named a variable the same name as a Visio Object - window. That can get confusing, but it's your call
Sub OpenVisioDrawing(ByVal filename As String, someData As String)
Private Function StartVisioInstance() As Object
Sub ActivateVisioInstance(ByRef app As Object)
Function LoadVisioDrawing(ByRef app As Object, ByVal filename As String) As Object
Sub SyncVisioDrawing(ByRef drawing As Object, ByVal someData As String)
Function DetermineDesiredVisioWindow(ByRef app As Object, ByRef drawing As Object) As Object
Sub ActivateVisioWindow(ByRef window As Object)
Sub MaximizeVisioWindow(ByRef window As Object)
Sub VisioScreenUpdating(ByRef app As Object, ByVal allow As Boolean)
All your functions are returning something and all of your subs are doing something, great! Your variable naming is consistent, but it's not all that descriptive.
I see a lot of ByRef
in there, and several that are implicitly ByRef
because they've not been designated ByVal
. Why are you sending everything as a reference? You're sending Objects
ByRef
into Functions
that return Objects
.
Class
I'm unclear as to why this class is existing. It doesn't seem you'll really need a factory for a bunch of Visio and I don't see any properties being stored in the class. I mean your class is essentially a Visio application object, if I'm reading this correctly (and I may not be).
Re: other
I ran this from Access 2016 with a sample vxd
and the Visio window never stopped blinking in the taskbar for a new instance of Visio. If it was already open, no blinking.
The blinking starts at
Set LoadVisioDrawing = app.Documents.OpenEx(filename, visOpenRW)
Itried moving ActivateVisioInstance
to after LoadVisioDrawing
but still, it blinks. I don't have a solution for that, I'm just being a negative nancy and pointing out problems :(
Stepping through the code though, still I don't understand how VisioInstanceExist
can possibly be True
without you calling, essentially AttachVisioInstance
.
I was wrong with my example of a new version of that function. It should be
Private Function GetVisioInstance() As Object
On Error GoTo NewInstance
Set GetVisioInstance = GetObject(, VISIO_CLASS_ID)
Exit Function
NewInstance:
Set GetVisioInstance = CreateObject(VISIO_CLASS_ID)
End Function
That's my fault, sorry. The point still remains though.
OpenVisioDrawing should be named better, but as far as what to name it, I can only give suggestions. VisioHandler, VisioInterfacing, VisioController, something more broad.
Re: Class
If I move all of the class outside to the test module and just
Sub Test()
OpenVisioDrawing "C:\Users\Ray\myVisioFile.vsd", "Custom drawing title"
End Sub
It works exactly the same as if it's in a class. Essentially your app
is your object, not your class. And an instance of Visio is an object. What I mean is, why is this a class if you aren't using any special properties? I'm not saying it shouldn't be a class, I'm saying think about why it's a class and what it being a class can do for you differently than if you just stick it in a module.
Sub Test()
Dim visioHandler As Object
Set visioHandler = New VisioInterface
With visioHandler
.OpenVisioDrawing "C:\Users\Ray\myVisioFile.vsd", "Custom drawing title"
End With
End Sub
Right, now we have an instance of the class that's an object outside of the class. What can we do with that?
Let's say in your class you put in
Private windowTitle As String
Public Property Get Title() As String
Title = windowTitle
End Property
Public Property Let Title(ByVal newTitle As String)
windowTitle = newTitle
End Property
Now you have something you can use outside of the class.
Sub Test()
Const PATH As String = "C:\Users\Ray\myVisioFile.vsd"
Dim visioHandler As Object
Set visioHandler = New VisioInterface
'Let .Title
visioHandler.Title = "My Title"
'Get .Title
MsgBox visioHandler.Title
'Get .Title
visioHandler.OpenVisioDrawing PATH, visioHandler.Title
End Sub
So instead of just sending an entire thing into the class to do everything, you can use those methods in the standard module.