HiveBrain v1.2.0
Get Started
← Back to all entries
snippetMinor

Excel VBA to parse JSON out of Google Maps API

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
googleexcelmapsparsevbajsonapiout

Problem

Some code that I wrote and broke into 5 separate questions on stack overflow. Some guy wanted to query Google Maps receive some JSON and then for each result found within that JSON do another query. I didn;t quite understand what he wanted at first and so actually there is some superfluous code here but I thought it would be useful to park my findings as to good practices for parsing JSON.

I use XHR requests. To parse the JSON, I use the ScriptControl. I have used this before. I have also seen a number of stack overflow questions which advocate third party libraries and I was puzzled by this. They all seem to take a mini-script approach of adding some javascript code to the script engine and calling it is cool but perhaps used too widely when there is a native solution available.

The key finding here is the use of VBA's CallByName function which can be used to query a JScriptTypeInfo Object Instance i.e. that which comes out of ScriptControl's Eval method.

CallByName can be used to get a member value; it can be used to query length of an array; it can be used to access elements of an array all with any javascript. Further I found some hasOwnProperty() method which allows defensive programming, so call this in cases where one thinks a member is missing. I also found some debugging sugar to stringify variables.

Look for some code review here. Will entertain suggestions, looking for best practice because in future looking to build J2EE application with REST interfaces that will use JSON and was planning to use Excel VBA as a debugging front end tool. Thanks.

```
'Tools->References->
'Microsoft Scripting Runtime
'Microsoft Script Control 1.0; {0E59F1D2-1FBE-11D0-8FF2-00A0D10038BC}; C:\Windows\SysWOW64\msscript.ocx
'Microsoft Xml, v6.0

Option Explicit
Option Private Module

Private Const sKEYNAME As String = "Server key 1"

'Public Const sKEY As String = "Your key goes here and uncomment"

Private Const sSEVENOAKS_PLACEID As String = "ChIJwd9bXUyt2Ec

Solution

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]=newI

Code Snippets

Public Sub DoSomething(ByVal foo As Integer, ByRef bar As Integer)
Public Sub DoSomething(foo As Integer, bar As Integer)
Public Sub DoSomething(foo As Integer, outBar As Integer)
Public Sub DoSomething(iFoo As Integer, iBar As Integer)
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

Context

StackExchange Code Review Q#131471, answer score: 4

Revisions (0)

No revisions yet.