Close RubberDuck Issues

[A9G-Data-Droid]

I used The RubberDuck VBA code inspection to perform a code review. I closed out all issues that made sense as of this writing. There should be no functional change in operation.

Replace generic functions with typed functions $ for String
Use VBNullString instead of ""
json_ParseErrorMessage returns a string
Pass parameters ByVal unless assigned a value ByRef
Always use Long instead of Integer. This prevents overflows on 64-bit systems and is better handled by modern CPUs.

[hecon5]

Looks good to me!

[jonadv]

About the (json_String As String, changes to (ByVal json_String As String,...

https://docs.microsoft.com/en-us/office/vba/language/concepts/getting-started/passing-arguments-efficiently

Passing an argument by value copies the original variable.
Notice the main methods ParseJson and ConvertToJson already pass the jsonstring ByVal. This prevents the original json element is changed within the JsonConverter module.

All arguments are passed to procedures by reference, unless you specify otherwise.
The passing by type is left out in the private functions, which sets it to ByRef.

Larger data types take slightly longer to pass by value than smaller ones. Because of this, String and Variant data types generally should not be passed by value.
As said, it's good that it creates a copy for the main methods ParseJson and ConvertToJson as it prevents the original json-element from being modified, but the (already copied) json_String should definitely not also be copied everytime it's passed in between the processing functions.

I would prefer the way it still is as its less code to read. If RubberDuck keeps bugging you with this, write ByRef where it is left out, fe (ByRef json_String As String,.

[A9G-Data-Droid]

Thanks for citing the documentation. You are correct, @jonadv. However in practice I've never seen a performance impact. I had this question myself when I first started using RubberDuck. The answer I got from Mathieu Guindon (the author and MS MVP for VBA) is that your code should say what it does. When you say something is ByRef you are telling the reader that you intend to change the value of the variable passed in. If you do not intend to change the value of the variable passed in you should make that clear by saying ByVal and thus the added verbosity is helpful to the human reader. So while this decorator is mainly for the users, not the compiler, you can also protect yourself from strange behavior and bugs by always being explicit in your declarations.

To keep the default ByRef for performance reasons is a premature optimization. You won't see the performance hit. If you have a truly hot path where a string is being copied ByVal in a loop then you could try passing ByRef to shave a millisecond off a long running operation.

The RubberDuck documentation for this inspection is here: https://rubberduckvba.com/Inspections/Details/ParameterCanBeByVal

[hecon5]

I agree with @A9G-Data-Droid; I strongly prefer explicitly stating which is which. There are many cases where I thought I was doing it one way (especially with inherited code), but the routines were acting on the passed in values instead of the output.

Explicitly stating which is which protects you from yourself, and protects your progeny from mishaps when they're on boarding to your project. While it might be the "right" way otherwise, I don't think any measurable performance impact outweighs the need.

[hecon5]

These must stay as Integer types; changing them to Long will break them and they will not contain the correct data.

[hecon5]

https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-systemtime for reference; the Word type is two bytes wide, and will result in incorrect handling otherwise.

[jonadv]

Anybody who cares to try out the performance impact or suggested changes with my self written benchmark tool? 😁

https://github.com/jonadv/VBA-Benchmark