Allow customized ranking statistics and sorting

[0spooky2me]

Possible implementation of #15

Implemented dropdown menus for choosing a stat to sort by and compare against any other stat
Moved some calculations out of UI code and into API code
Slightly improved test code, but it can be removed

[amrocha]

I don't think this will break anything, left a bunch of comments regarding readability but up to you if you want to fix them

I didn't do a manual test or anything though

[amrocha]

wow nice automated docs

[amrocha]

This works but I bet you could really simplify it with a more suited data structure here. I'm not super familiar with the code base so I can't tell you the optimal solution, but off the top of my head I think that the following structure would lead to more readable and resilient code:

{
  japanese: {
    ...
  },
  hongKong: {
    ...
  ,
}

Resilient because string manipulation like this is prone to breaking during refactors, and since we're not using any typing here JS is gonna fail this silently and we might not notice.

[amrocha]

I do think this is better than before though

[0spooky2me]

I agree with this, I was thinking of making this change. Should I do that in this PR?

[amrocha]

follow-up PR is fine, this is pretty big already

[amrocha]

oh my god I hate that this line is even allowed to work

[0spooky2me]

Welcome to Meteor.js

[amrocha]

Why disabled? Why are there two ELO options actually?

[0spooky2me]

I couldn't think of a good UX solution to indicate that the sortStatistic column is where sorting comes from. How do you indicate that changing this option will re-sort?

[amrocha]

oh huh, idk it seems pretty intuitive to me but if it's not obvious enough this is fine, maybe change the wording to Sort by ... or something

[amrocha]

kinda confusing that statisticsList[0].format is the format for the stat, but format is the type of game

[0spooky2me]

That's fair.

[amrocha]

Can simplify this as

statisticsList = [
  {value: "averageHandDora", format: "Avg Hand Dora"},
  {value: "riichiRate", format: "Riichi %"},
  {value: "riichiWinRate", format: "Riichi Win %"},
  ...statisticsList,
];

[amrocha]

Or even

statisticsList = format === Constants.GAME_TYPE.JAPANESE ? [
  {value: "averageHandDora", format: "Avg Hand Dora"},
  {value: "riichiRate", format: "Riichi %"},
  {value: "riichiWinRate", format: "Riichi Win %"},
  ...statisticsList,
] : statisticsList;

but maybe this is too clever

[0spooky2me]

Your Javascript powers are too strong for me

[amrocha]

are you one of those people that use for loops in js? I thought you were better than this

[amrocha]

Anyway this can be simplified to

statisticsList.filter((statistic) => !excusions.includes(statistic));

Combined with the previous simplification this function becomes something like

function getRankingStatistics(format, exclusions) {
  let standardStatisticsList = [
    {value: "elo", format: "ELO"},
    {value: "gamesPlayed", format: "Games"},
    {value: "handWinRate", format: "Hand Win %"},
    {value: "dealinRate", format: "Deal-in %"},
    {value: "averageHandSize", format: "Avg Hand Size"},
    {value: "averagePosition", format: "Avg Position"},
    {value: "flyRate", format: "Bankrupt %"},
    {value: "chomboTotal", format: "Chombos"}
  ];
  const japaneseStatisticsList = [
    {value: "averageHandDora", format: "Avg Hand Dora"},
    {value: "riichiRate", format: "Riichi %"},
    {value: "riichiWinRate", format: "Riichi Win %"}
  ];

  return [...standardStatisticsList, japaneseStatisticsList].filter(
    (statistic) => (
      !exclusions.find((exclusion) => exclusion.value === statistic.value) && 
      (japaneseStatisticsList.includes(statistic) && format === Constants.GAME_TYPE.JAPANESE)
    );
}

I think it reads easier, up to you though

did not test this code btw make sure I didn't fuck up somewhere if you use it

[0spooky2me]

I need time to process this comment. You're probably right though. I thought Javascript was all about that list comprehension though? Are for loops actually discouraged?

[amrocha]

Some people like for...each, but generally the React community tend to favor the functional approach with map etc. and I work with React everyday so I don't like for loops.

List comprehension might be a thing more recently, but generally the JIT compiler is so good now that performance is gonna be roughly the same no matter what you use

[amrocha]

Are these the only places where exclusions can come from?

In that case I recommend having a list of possible statistics, and then picking items from it like so

const STATISTICS_LIST = [...];
additionalRankingExclusion() {
  return STATISTICS_LIST[2];
}

or something like this, it'll make the comparisons easier to read since you'll be able to just compare references to objects instead of the inner content of these objects. It's also more performant but that's whatever.

It's kind of a big change and this is fine though

[0spooky2me]

I'm going to leave this for now.

[amrocha]

I have no idea what's going on here lmao

[0spooky2me]

Apparently this is the syntax
'change -> Detect a "change event"
select -> on an HTML select field
[name="sortStatistic"]' -> with the name "sortStatistic"
(event) -> capture the event

[amrocha]

I'm like, super confused how this is calling a function with a string name? Or I guess this is a function definition. JS is weird

[0spooky2me]

JS is the worst

[amrocha]

can simplify this as

return ["handWinRate", "dealinRate", "flyRate", "riichiRate", "riichiWinRate"].includes(statistic) ? '%' : '';

[0spooky2me]

Addressed PR comments, please re-review @Mooters @amrocha

[amrocha]

good stuff