5
\$\begingroup\$

I have a dictionary of parameters and value, and the function creates a URL query string from it:

Private Function _BuildUrlFromPermalinkArgumentDictionnary(ByVal oParameters As Dictionary(Of String, String))
    Dim returnString As String = ""
    For Each kvp In oParameters
        returnString += kvp.Key + "=" + kvp.Value + "&"
    Next
    Return Left(returnString, Len(returnString) - 1)
End Function

For example, the input dictionary

A -> 12356
B -> foo
C -> bar

have the output A=12356&B=foo&C=bar

I would like to improve this, not really for performance, because the dictionary is small enough that it will be really fast no matter what, but as an opportunity to learn what I could have done in a better way.

In particular, the way I remove the trailing & feels wrong.

\$\endgroup\$
1
  • \$\begingroup\$ Perhaps you have already determined this won't be an issue in practice, but consider what to do on various bad inputs: values with ampersands or equal signs, null objects, empty strings. Also, whether it matters if a slash will be interpreted as part of a string or as a URI path. And whether duplicate keys are a problem or not. Just some things to think about. \$\endgroup\$
    – BurnsBA
    Sep 5, 2017 at 15:32

2 Answers 2

5
\$\begingroup\$

This isn't a bad approach - simple, effective, and gets the job done. It could be ever-so-slightly better, though.

First, string concatenation (str1 & str2, str1 + str2) in .NET has been known to be extremely slow at times, due to extra allocations. (Basically, str1 + str2 + str3 + str4 creates a temp string str12, then creates another temp string str123 and then creates a final string str1234. Doing this in a loop means that it creates a temp string returnStringStr1234 then sets returnString to that, and then repeats all four of these new allocations for each iteration, at a minimum.) There's something in the framework called StringBuilder, and I'd like to recommend you look into it.

I should also mention that we also use StringBuilder from a semantics point of view: instead of wondering what returnString does (is it a bunch of sub-strings, is it just one string being set by a certain loop iteration, etc.) we have something that indicates that our return value should be building a string. Everything starts to make sense.

Essentially, the StringBuilder has a sort of internal buffer that, instead of creating a new copy of the strings, it just adds the strings together. This also exposes a few useful properties, one of which being Length, which can be increased or decreased, removing the need for Left(...).

Dim result As New StringBuilder()
For Each kvp in oParameters
    result.Append(kvp.Key).Append("="c).Append(kvp.Value).Append("&"c)
Next
result.Length -= 1
Return result.ToString()

This is only one line-of-code longer than your version, but it should perform more quickly and is also clearer about what the last step is (remove the last character). Of course, an alternative implementation that avoids manipulating the StringBuilder like that might look like:

Dim result As New StringBuilder()
For Each kvp In oParameters
    If result.Length > 0 Then result.Append("&"c)
    result.Append(kvp.Key).Append("="c).Append(kvp.Value)
Next
Return result.ToString()

In this situation we test if the Length is > 0 to see if it already has an element. If so, we append the ampersand ("&"c) and continue on. Now we don't need to worry about the last character being an ampersand at all.

I've used a couple constructs that may be unfamiliar to you:

  1. The If expression Then statement is a single-line If statement. It must be on one line (unless in a line-continuation context) and will do the same thing as If expression Then <NEWLINE> statement <NEWLINE> End If, but is much more succinct. I use them for guard-clauses like this a lot.
  2. The "_"c construct creates a character literal from the specified string (which should be one character only). The c is the magic bit, you can remove it from everything if you like.

I would also recommend adding the explicit type for your kvp:

For Each kvp As KeyValuePair(Of String, String) In oParameters
    ...
Next

This makes it much more clear to the compiler as to what type you expect, and tells future programmers that you do know what you're doing.


Overall, good work. Hopefully this is somewhat educational for you and not just a bunch of random nonsense. :)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot. A few things I knew, a lot I learnt, definitely bookmarked :) \$\endgroup\$
    – Maxime
    Sep 5, 2017 at 15:02
1
\$\begingroup\$

The most valuable tip anyone can give you is to learn about the strict compiler option. When set to On the compiler restricts implicit data type conversions to only widening conversions, disallows late binding, and disallows implicit typing that results in an Object type. No more "Cat" + 8! :) Exhibit A

It's also worth mentioning that the Microsoft.VisualBasic assembly is packed with backward compatible (< VB7) stuff, like the good ol' Left and Len method. If there's an equivalent or a similar subtitute in the System namespace, use it. In doing so, you'll find it easier to read/write any of the other .NET languages.

Below is a simple and readable solution using string join, linq, lambda expression and string interpolation.

Option Strict On

...

Private Function BuildUrlFromPermalinkArgumentDictionary(ByVal parameters As Dictionary(Of String, String)) As String

    Return String.Join("&", parameters.Select(Function(pair) $"{pair.Key}={pair.Value}"))

End Function

.NET fiddle

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.