PayloadGeneratorTest.cs Test Failure

[csturm83]

Type of issue

[X] Bug
[ ] Question (e.g. about handling/usage)
[ ] Request for new feature/improvement

Expected Behavior

All tests pass.

Current Behavior

swissqrcode_generator_should_throw_wrong_countrycode test fails after commit bbba9cba

Steps to Reproduce

Run tests on latest version of master branch.

[csturm83]

Actually, I think this has been around for a while, but just noticed it when testing all targets. This line doesn't necessarily enforce length of 2 for the country string passed in.

I'm not familiar with the spec for this payload. If it strictly requires country code of length 2, then this is a bug. Otherwise, the test should be updated to pass on all targets.

Either way, I think the behavior should be made consistent across targets.

Related: #208

[codebude]

This is fixed in https://github.com/codebude/QRCoder/commit/7e6cc9284383f13f3b49c49461acc2c584684227 Since .NET still doesn't bring a complete list/all-in-one solution for the ISO codes, I decided to add an external solution. If you disagree with the code changes, feel free to re-open the issue. :-)

[csturm83]

I'm not a huge fan of the additional Package Reference for all build targets. Prior to this change, the only build target to have an external Package Reference was netstandard2.0 (System.Drawing.Common), which seems minimal.

This ISO3166 package essentially boils down to one file. Why not just import this single file periodically with the proper license header, instead of a Package Reference? Minimal dependencies is a HUGE selling point, in my opinion.

[csturm83]

Also, looking at an open PR in that repository, the package/dll size could potentially increase in the future for a localization feature we likely don't need or care about.

[codebude]

That's a good argument. Maybe the solution I implemented wasn't well thought. Feel free to add a merge request with a static list/export of the ISO codes. (Otherwise I'll see that I rework the code next week.)

[codebude]

Hi @csturm83 - I rewrote the code. Let me know if this is a better solution: https://github.com/codebude/QRCoder/commit/822f73925c0d2b3850c61272602aefd7d0d4bc36

[csturm83]

Yup, looking better.

1. Maybe use ToUpperInvariant instead of ToUpper? In C# what is the difference between ToUpper() and ToUpperInvariant()?
2. If you care about performance (not sure it matters with all the Regex code that precedes it) a. Use a pre-sized HashSet (O(1) vs. O(n) w.r.t. Contains). b. Don't recreate the data structure on every method call (cache it statically)

[codebude]

Hi @csturm83 Thanks for the hints.

1. I'll change the code to ToUpperInvariant
2. I guess there are a lot of things to optimize from a performance perspective. It's not that I don't care about performance, but when initially writing the library, I focused on building a running solution. And it's true, that there is a lot of potential for optimization and maybe I'm not experienced enough to know/see every possible performance tweak. But if you have suggestions for optimization, I'm really happy to implement them. a. Do you mean that I should use var hashSet = new HashSet<string>(2) { "AF", "AL" }; instead of string[] codes = new string[] { "AF", "AL" }? From what I got from here https://stackoverflow.com/questions/6771917/why-cant-i-preallocate-a-hashsett is that this is a feature of .NET 4.7.2. Since the library is build to "older" targets I'm not sure if it's a good idea to use it. Or did I misunderstood you? b. What do you mean by "cache it statically"?

[csturm83]

@codebude by "cache it statically", I simply meant to create/store it in a private static readonly variable. Maybe on the Contact class, unless you would ever need these codes in other Payloads? That way the data structure is only allocated once, and reused across IsValidTwoLetterCode method calls.

Bummer about the HashSet constructor overloads prior to 4.7.2. Fortunately, using the default HashSet constructor would only be a minor difference when instantiating the HashSet object (one time cost, if static). The main reason you might consider HashSet over an Array is with respect to Contains.

All that said, performance in this particular spot of code is probably not that critical. cc @jnyrup

[jnyrup]

I would probably write something like

private static readonly HashSet<string> twoLetterCodes = ValidTwoLetterCodes();

private static HashSet<string> ValidTwoLetterCodes()
{
    string[] codes = new string[] {...};
    return new HashSet<string>(codes, StringComparer.OrdinalIgnoreCase);
}

private static bool IsValidTwoLetterCode(string code) => twoLetterCodes.Contains(code);

The constructor taking an IEnumerable<T> will check if it is an ICollection<T> and use its Count property to set the capacity. At least for .net 4.8 https://referencesource.microsoft.com/#System.Core/System/Collections/Generic/HashSet.cs,144

Passing in StringComparer.OrdinalIgnoreCase makes the Contains method do case-insensitive comparisons.

[codebude]

Hi @csturm83 , @jnyrup - thanks for your input. I followed your suggestions here: https://github.com/codebude/QRCoder/commit/cdfe472520a30615758a7b021613bf9cfcad0192