CodeSOD: Uniquier

Sole Purpose of Visit's team had a simple enough problem: they needed to generate unique labels for widgets on the screen. These widgets just need those unique IDs for internal purposes, so a simple incrementing counter would have been sufficient.

That isn't the direction they went in.

namespace Initrode.Widget{ /// <summary>/// Requirement: we need a string of length 22 for a widget id /// Note changed from static to instance to allow for TPL. /// </summary>/// <remarks>/// Was static. Changed to non-static class so that it works with Parallel.ForEach/// </remarks> public class Labeller { public string UniqueValue {get{StringBuilder sb = new StringBuilder();byte[] bytes = Guid.NewGuid().ToByteArray();sb.Append(Convert.ToBase64String(bytes));// Make sure we don't include characters that will interfere with later string outputsb.Replace('/', '&');sb.Replace('+', '#');sb.Remove(22, 2);return sb.ToString();} } }}

The initial comment lies: they don't need 22 character strings. This isn't a requirement. Further, the remarks lie: there's nothing about making it non-static that would be required for using Parallel.ForEach.

But let's set aside the lies. They've opted to base their unique ID off a GUID. While this is arguably overkill, it's not wrong, and it does have the benefit of parallelizing nicely. But everything else about this is bonkers.

First, they generate the GUID, then convert it into a byte array. Then they base-64 encode the byte array. But this means that there'll be some characters they don't want to use (for some reason), so they have to find-and-replace to remove some of those characters. Then we delete 22 characters from the Base64 encoded GUID.

Now, again, there's no requirement for character length, and clearly they're free to use a string representation, so a simple Guid.NewGuid().ToString() would have absolutely sufficed, even if that itself is overkill for the problem at hand. But there's no logic behind why they base-64 encode the bytes, no reason why they need to replace some characters, and no reason why the method can't be static. In fact, the fact that it's not static leads to this calling convention:

var value = new Labeller().UniqueValue

This constructs a class for no reason, and thus entails all the overhead of creation, destruction, and eventual garbage collection.

"Sole Purpose of Visit" has this to say about the submission:

What I like about it is that it is a very, very, short piece of code that is easily understood. Oh, and also that every single line strikes me as stupid.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.