invertedtomato / crc

A flexible CRC implementation supporting most major algorithms. Also includes ability to support custom implementations.
MIT License
29 stars 10 forks source link

Extension API could be enhanced like Convert.ToHexString and improved in terms of speed #10

Closed maettu-this closed 1 year ago

maettu-this commented 1 year ago

Hi Ben,

The HexStringExtensions is what Convert.ToHexString is for .NET 5+. Extending the API with e.g. the (Byte[], Int32, Int32) overload would make it even more useful.

In terms of speed, the analysis in https://stackoverflow.com/questions/311165/how-do-you-convert-a-byte-array-to-a-hexadecimal-string-and-vice-versa/624379#624379 indentifies https://stackoverflow.com/questions/311165/how-do-you-convert-a-byte-array-to-a-hexadecimal-string-and-vice-versa/24343727#24343727 as a much faster approach than the current BitConverter based approach.

These are just ideas. Feel free to accept or reject.

Best regards, Matthias

maettu-this commented 1 year ago

Another minor: target.ToString("X").Replace("-", "") can be simplified to target.ToString("X") as UInt* values can never be negative.

invertedtomato commented 1 year ago

Hi @maettu-this! Thanks for the thoughts. ToHexString is only used for debugging and automated testing and hasn't been intended for any intensive use. If you were to raise an optimisation PR that didn't involved upgrading to .NET 5 (as discussed in the other issue) and remained "safe" I'd be happy to accept it, though I couldn't justify spending time on it myself at this stage.

target.ToString("X").Replace("-", "") can be simplified to target.ToString("X") ToString("X") puts hyphens between bytes (ie 00-00-00) and this is to remove that. It's not there to deal with negative integers.

maettu-this commented 1 year ago

@invertedtomato, target.ToString("X") does put not hyphens, see https://dotnetfiddle.net/v1zQXt.

invertedtomato commented 12 months ago

So it is, when ToString("X")ing a *int it does not add hyphens. Hyphens are only used on byte arrays. I've pushed a change for this now.