Fix the order of the map

[PumpkinSeed]

Resolve #125

I wrote a custom sort implementation this scenario. I know that by the documentation it shouldn't have letters in that key, but I wanted to handle that case as well, where the map's key is a letter or non-numeric.

Benchmarks:

// Previous solution with sort.Strings(...)
//         BenchmarkOrderedMap_MarshalJSON-8         250574          4697 ns/op        1329 B/op          46 allocs/op
// New solution with custom sort algorithm
//         BenchmarkOrderedMap_MarshalJSON-8         211605          5277 ns/op        1707 B/op          53 allocs/op

I think there can be a few performance improvements especially for the strconv.ParseUint.

[alovak]

@PumpkinSeed, thanks for the PR! Great improvement!

There's a similar implementation in the Describe method here: https://github.com/moov-io/iso8583/blob/master/describe.go#L161

It would be great if we could use just one implementation. Should we make the type exportable?

I also appreciate the benchmark tests you've created!

[PumpkinSeed]

What do you think of put it into the utils package?

Also I prefer the strconv.ParseUint over the strconv.Atoi, because the Atoi calls the ParseInt, and ParseInt calls the ParseUint. So there is a very small performance upgrade.

Please let me know your thoughts on it.

[alovak]

I'm a bit late with response. We already have sortpackage which is public.

What if we:

• use alphaNumericSlice name instead of Numeric as it sorts both and it follows similar convention as IntSlice, StringSlice; keep is private but inside sort package
• create sort.AlphaNumeric that will use alphaNumericSlice

@PumpkinSeed @adamdecaf thoughts?

[alovak]

I'll also think about how to solve the usage of panic so linter is happy.

[PumpkinSeed]

I'm a bit late with response. We already have sortpackage which is public.

What if we:

• use alphaNumericSlice name instead of Numeric as it sorts both and it follows similar convention as IntSlice, StringSlice; keep is private but inside sort package
• create sort.AlphaNumeric that will use alphaNumericSlice

@PumpkinSeed @adamdecaf thoughts?

I tested it and the StringsByInt doing exactly the same what we need. So I have two proposal. In both case I can remove the Numeric struct implementation.

1. If we can assume that the keys never will be alphabetic rather only numeric we can use it. Based on the spec of iso8583 I can safely say that it won't be alphabetic, but please correct me if I'm wrong.

2. Same as above, but I can add return x[i] < x[j] instead of the panic. In this case we will have the same functionality IN THEORY. All of the tests passes. Also that will resolve the panic issue for the function.

[alovak]

I'm leaning towards 2nd option and not using panic at all. It looks like a breaking change, but I think it should have no impact on real integrations.

[PumpkinSeed]

I'm leaning towards 2nd option and not using panic at all. It looks like a breaking change, but I think it should have no impact on real integrations.

I checked it and mostly it's used by tests. I will check the others and see what can happen in case of this change.

[PumpkinSeed]

Or have something like this:

func StringsByInt(x []string) {
    sort.Slice(x, stringsByInt(x, func(i, j int) bool {
        panic("failed to sort strings by int: failed to convert string to int")
    }))
}

func StringsByIntOrString(x []string) {
    sort.Slice(x, stringsByInt(x, func(i, j int) bool {
        return x[i] < x[j]
    }))
}

func stringsByInt(x []string, fn func(i, j int) bool) func(i, j int) bool {
    return func(i, j int) bool {
        valI, err := strconv.Atoi(x[i])
        if err != nil {
            return fn(i, j)
        }
        valJ, err := strconv.Atoi(x[j])
        if err != nil {
            return fn(i, j)
        }
        return valI < valJ
    }
}

but it feels like overengineering the problem.

---

It is used in these places:

• examples/VSDCChipData/main.go
• specs/builder.go

In the specs the tagDummy struct has a sort field which used for sorting purposes. Let's say I implement the 2. option for now and we will see.

[PumpkinSeed]

Also, there are 5 panics left in the code. What can I do with them to satisfy the linter? I would recommend the new slog package, but the users of the library wouldn't be happy if they just randomly getting logs after a package update instead of panics what they are prepared for.

[alovak]

You can ignore linter for now. I'll create a separate PR. I'll either ignore linter alerts for panic or rework some func/method signatures.

[PumpkinSeed]

You can ignore linter for now. I'll create a separate PR. I'll either ignore linter alerts for panic or rework some func/method signatures.

If you create an issue I can work on that part.

[alovak]

@PumpkinSeed here it is: https://github.com/moov-io/iso8583/issues/269. Thank your for your help!

btw, are you in Moov's Slack channel about #iso8583? If no, join https://moov-io.slack.com/archives/C014UT7C3ST

I would want to know more about how and for what you are using iso8583 :)

[adamdecaf]

Thanks for the PR! The linter is from a separate improvement we're making across all of our repositories.