sort vowels to the end of the trackers animation to avoid dodgy words

[brindy]

Task/Issue URL: https://app.asana.com/0/392891325557410/1179212544274947
Tech Design URL:
CC:

Description:

Sorts the icons to show so that vowels are at the end of the list to avoid dodgy words being spelled.

Steps to test this PR:

1. Visit imgur.com - the tracker icons that appear should spell GYA (A might be Amazon's logo)
2. Browse a few other sites and confirm no breakage

---

Internal references:

Software Engineering Expectations
Technical Design Template

[marcosholgado]

FYI: This should also be tested against the trackers dialog. We should be showing the same trackers that we display in the animation. I believe imgurl only has 3 trackers so it will always work for this case.

[CDRussell]

displayName can't be null, but could it ever be an empty string? if so, that line would crash.

displayName.take(1) is safer in this regard, and we use that elsewhere in the same function so is also more consistent. Happy to switch displayName[0] for displayName.take(1) ?

Also, would be great if there were some unit tests for this comparator if it's easy to pull that out into something testable

[marcosholgado]

displayName can't be empty, we are already filtering blank names out in Site.kt with .filter { it.displayName.isNotBlank() }

[cmonfortep]

LGTM, I proposed a small improvement but optional.

[cmonfortep]

Just a suggestion. I think at some point we may not remember why this logic was here, and since we don't have tests for this class (document purposes), I suggest to create a method so we can make it more descriptive.

We could just create a extension function for Sequence<Entity>, it will look like this:

    private fun Sequence<Entity>.sortToAvoidDodgyWords(): Sequence<Entity> {
        return sortedWith(compareBy { "AEIOU".contains(it.displayName[0]) })
    }

and yout code will change in this way:

(...)
.take(MAX_LOGOS_SHOWN + 1)
.sortToAvoidDodgyWords()
.map {
(...)

[CDRussell]

But we can avoid the use of the word dodgy? sortToAvoidSpellingWords() perhaps

[cmonfortep]

sure, I was just using @brindy 's word as an example :D

[brindy]

Thanks for the comments - I don't think this is easy to extract for testing without adding a lot of extra stuff.

I've made the change for the take(1) (thanks @CDRussell ) and extracted it to a private sequence extension (thanks @cmonfortep )