Return to Answer

• 1

• 75.5k
• 18
• 194
• 467

I'll clear the easy stuff first, using Rubberduck 2.0b code inspections:

Language Opportunities

• Prefer vbNullString to "": The built-in constant vbNullString is a null string pointer taking up 0 bytes of memory, that unambiguously conveys the intent of an empty string.
• Use of the obsolete Call statement: The Call statement is no longer required to call procedures, and only exists in the language to support legacy code that did require it. It can be safely rewritten to the more modern implicit call form.

Maintainability and Readability Issues

• Consider renaming variable sType0: identifier names should indicate what they're used for and should be readable. Avoid numeric suffixes.

Code Quality Issues

• Constant sKEYNAME is not used. Consider removing it.
• Return value of function TestAll is never used. Consider making the function a Sub procedure instead.
• Return value of function ParseTextSearchResponse is never used. Consider making the function a Sub procedure instead.
• Return type of function ParseTextSearchResponse is implicitly Variant - apparently that function was actually meant to be a Sub.
• Return value of function ParseTextSearchResponse is never assigned. That's it, it's a Sub in a Function disguise!
• Parameter vDefaultValue (in GetJSONPrimitive) is implicitly passed by reference. Consider making it explicitly ByRef.
• Parameter vDefaultValue could be passed by value... unless it could be an array? This inspection result comes up because the parameter isn't assigned a new value in the function's body, but if an array is a valid value for it, then passing it ByVal would break the code. If an array isn't a valid value for it, then passing it ByVal would make the intent clearer.
• Return type of function NearbySearch is implicitly Variant. Yet you're assigning it a Scripting.Dictionary - why not specify the return type?
• Function TestAll is not used. And it's Private, too - which makes it essentially unreachable.
• Variable sTopPrediction is never used in EvenBiggerTest. It's assigned a value, but that value serves no apparent purpose.
• Variable dicDetails is never used in TestTextSearch. Again it's assigned, but nothing is done with the assigned value.
• Variable sName is never used in ParseTextSearchResponse. Assigned from a call to GetJSONPrimitive, and then nothing.
• Variable dicWeekdayKeys is never used in ExtractOpeningHours.
• Variable lTypesLength is not used either, in PlaceDetails.

Not bad at all, I've seen shorter code trigger more inspection results than that!

###Hungarian Notation

Your naming style consistently (good!) uses a heavily discouraged (bad!) Hungarian Notation that encodes a variable's type into its identifier name, which hurts readability (lowercase "L" for "Long"? that's plain evil!) with zero benefits, especially since you're declaring variables as close as possible to their usage, so the variable's type is right there in your face anyway - kudos for avoiding the unfortunately too common "wall of declarations" trap!

The "right" way of using Hungarian Notation, is to add meaningful context - the name of the type of a variable isn't meaningful context. Read Making Wrong Code Look Wrong on Joel on Software for the whole argumentative and examples of "Hungarian Notation done right".

Applied to VBA, I like to use ByRef parameters to illustrate. Consider this signature:

Public Sub DoSomething(ByVal foo As Integer, ByRef bar As Integer)

Ignore the fact that bar could be the return value of a Function for a moment - this is just an example. What clue does the user of this procedure have that bar is really an out parameter? None. And we're lucky here, we have explicit ByVal and ByRef modifiers. Imagine this signature for a procedure that does exactly the same thing:

Public Sub DoSomething(foo As Integer, bar As Integer)

Ew. Now consider this:

Public Sub DoSomething(foo As Integer, outBar As Integer)

Oh. An out prefix tells us that the second parameter is actually a return value! That is a useful prefix. Compare to:

Public Sub DoSomething(iFoo As Integer, iBar As Integer)

The i-for-Integer prefix is totally redundant and useless.

GetScriptEngine calls AddCode 5 times, but once would be enough:

Private Function GetScriptEngine() As ScriptControl
 Static scriptEngine As ScriptControl
 Static script As String
 
 If scriptEngine Is Nothing Then
 
 script = GetJavaScriptLibrary("https://raw.githubusercontent.com/douglascrockford/JSON-js/master/json2.js") & _
 "function getKeyValues(jsonObj) { " & _
 " var dictionary = new ActiveXObject(""Scripting.Dictionary""); " & _
 " var keys = new Array(); for (var i in jsonObj) { dictionary.add(i,jsonObj[i]); }; return dictionary; } " & _
 "function setKeyValue(jsonObj, key, newItem) { jsonObj[key]=newItem; return jsonObj; }" & _
 "function toVBString(jsonObj) { return JSON.stringify(jsonObj); }" & _
 "function overrideToString(jsonObj) { jsonObj.toString = function() { return JSON.stringify(this); } }"
 
 Set scriptEngine = New ScriptControl
 scriptEngine.Language = "JScript"
 scriptEngine.AddCode script
 End If
 
 Set GetScriptEngine = scriptEngine
 
End Function

I have to say I'm ambivalent about Static locals: they have that funky smell - a Static local could very often just as well be declared at module scope... but then if they're only ever used in one place, what's the point, right?

I think your module is doing too many things, and that you need more objects in your life. Class modules. I'd move that to a ScriptEngine class, have the script live there, encapsulated, and holding on to its ScriptControl instance, perhaps even encapsulate that as well, and only expose methods that the client code needs to see.

Ignoring the assignment of the function's return value, it's only used in 4 places:

all references to 'GetScriptEngine' - Rubberduck "search results" docked toolwindow

The members used are Run and Eval - so yeah, I'd encapsulate it and expose a Run and an Eval method for the client code to consume, instead of relying on Static locals and procedural code.

Here's another example of poor naming:

Dim vKeyLoop As Variant
For Each vKeyLoop In dicKeys.Keys

vKeyLoop tells you it's a dictionary key... and that it's used in a loop. Oh and that it's a Variant.. If you're going to iterate strings in a For Each loop, it needs to be a Variant - so again the Hungarian prefix serves no purpose at all. But that's not why it's a bad name: the real question is, what does the darn key represent? THAT is what, as a maintainer of that code base, I'd like to be able to infer from the variable's name. I know it's a key because I'm iterating keys. I know it's a loop variable because, well, it's a For Each loop variable anyway. And I know it's a Variant because it has to be a Variant for the For Each loop to even compile.

Speaking of compiling... sKey isn't declared in BigTest, which, because of Option Explicit, makes the code uncompilable. Ditto in EvenBiggerTest, and TestTextSearch as well.

Speaking of testing... The test code should really be separate from the code it's testing. Pull these methods into their own module, it's urgent!

The tests themselves aren't clear about what the expectations are; they merely output stuff, but we don't know if it's right or if it's wrong until we visually inspect the ActiveSheet and interpret the results.

If you could break the coupling with the web API, extract the functionality into a class that only exposes it (the functionality) via an interface, then you could implement a mock version of that interface and write (and run!) actual unit tests, which would test all the functionality without ever hitting the web (i.e. they would run in a few milliseconds), and you would have an immediate feedback about exactly which part of the spec you broke with your latest modification - if that sounds interesting, I suggest you take a look at the features page of Rubberduck's website.

Disclaimer: I'm totally, completely, heavily involved with the Rubberduck project.