Optimization: decode a single ID without allocating an array

[michaldivis]

Hi,

I'd like to propose an optimization for the Decode method. I'll be decoding a single ID most of the time, so it might make sense to add add the following methods:

//decode a single int ID from a hash
int DecodeSingle(string hash);

//decode a single long ID from a hash
long DecodeSingleLong(string hash);

This approach would not require allocating an array for returning single values.

[ullmark]

Yeah, makes sense I guess... The methods originally were meant to match what they were called and how they work in the javascript library, but for instance, the concept of long was something added that javascript doesn't support.

I think that first and foremost a library should be easy to use for the developers. Micro optimizations in performance, memory usage etc is less important. It is very unlikely that "Hashids" will be the bottleneck in an application.

For instance it would be more "true" to return uint since the library doesn't suport negative numbers, but I dropped that idea since I've never actually used that myself when working with IDs so didn't want to force ppl to do casting.

I guess the issue with this would be that you tell the library to make something into a single value that can be multiple, what should the library do then? Throw error or just give you the first value?

[michaldivis]

Thanks for such a quick answer. I understand that the minor memory usage optimization isn't worth adding more complexity to the library.

To answer your question:

I guess the issue with this would be that you tell the library to make something into a single value that can be multiple, what should the library do then? Throw error or just give you the first value?

I'd consider that being an error. This new method would be only expecting a single value hash so a has containing multiple values could be considered invalid.

Another reason I think this proposal might make sense is that it would make the usage more explicit about only expecting one number. I've' always been using this library to translate a single hash to a single entity ID and my proposed solution might be more readable and explicit in that use case.

If you don't mind, I'd like to leave this proposal here up for debate just in case anyone else is interested.

How I'm using the current implementation:

private Book GetItemByHashUsingDecode(string hash)
{
    var ids = _hashids.Decode(hash);

    if(ids.Length == 0)
    {
        // error: invalid hash, not very explicit
    }

    if (ids.Length > 1)
    {
        // error: hash is contains more than one ID, not very explicit
    }

    var firstId = ids.FirstOrDefault();

    return _bookService.FindById(firstId);
}

Proposal

I'm doing the same thing in two different ways here, what do you think about them? Which one would you prefer?

Option 1: DecodeSingle with possible exceptions

public interface INewHashidMethods
{
    /// <summary>
    /// Tries to decode a hash into a single ID, throws an exception when the hash is invalid or contains more than one ID
    /// </summary>
    /// <exception cref="InvalidHashException" />
    /// <exception cref="MoreThanOneIdInHashException" />
    int DecodeSingle(string hash);
}

Usage:

private Book GetBookByHashUsingDecodeSingle(string hash)
{
    try
    {
        var id = _newHashids.DecodeSingle(hash);
        return _bookService.FindById(id);
    }
    catch (InvalidHashException ex)
    {
        // invalid hash
    }
    catch (MoreThanOneIdInHashException ex)
    {
        // hash is contains more than one ID
    }
}

Option 2: TryDecodeSingle

public interface INewHashidMethods
{
    /// <summary>
    /// Tries to decode a hash into a single ID
    /// </summary>
    bool TryDecodeSingle(string hash, out int id);    
}

Usage:

private Book GetBookByHashUsingTryDecodeSingle(string hash)
{
    var success = _newHashids.TryDecodeSingle(hash, out int id);

    if (!success)
    {
        // hash is invalid or contains more than one ID
    }

    return _bookService.FindById(id);
}

[manigandham]

Hey everyone, sorry about the delay here. Thanks for the suggestion @michaldivis, this has come up before so two points about the history:

• There's no performance benefit because the array is already allocated to hold the result in the first place, so saving on the return won't matter. There are some low-level optimizations like passing in your own buffer or adding a flag throughout the encoding process to exit early but this would massively complicate the code, and the savings would be insignificant since there are plenty of other arrays and buffers used internally. The library has already been heavily optimized in the past few months so it shouldn't be a bottleneck anymore.

• Hashids is actually a widely implemented "standard" with libraries in about 40 other languages. I didn't want to stray too far away from the universal API and possibly confuse users about the intent and meaning of the functions so the array checks were left to the user's implementation.

However the ease of use for this common scenario makes sense and clearly the community wants this so I think we can make it work with the idiomatic TryDecode... pattern. I'll check on PR #57 by @Cereal-Killa and get it merged in soon.

[manigandham]

• New version 1.5.0 has been released: https://www.nuget.org/packages/Hashids.net/1.5.0
• Changelog with several big improvements: https://github.com/ullmark/hashids.net#changelog

Thanks for everyone's help with this!