Makes dates increase in difficulty, keeping them 5 years apart at all times

[benguaraldi]

I believe this addresses #19 and #8 (though there was code for #8 in there already) by requiring the first two cards are 100 years apart, then 90, 80, 70, ... 20, 10, and finally 5 for the rest of the game. (i.e., it never allows the cards to be closer than 5 years together.)

The loop that compares the dates is more processor intensive of course, but I think it should still be pretty fast.

[karenerobinson]

This is cool, great improvement. in my opinion the cards should be farther apart when they are older.

[benguaraldi]

@karenerobinson Thanks!

Let's get this one implemented, but if @tom-james-watson agrees, maybe another PR that makes cards further apart when they're older is in order.

[benguaraldi]

Thanks, @tom-james-watson! I think I've fixed that bug, but I did it by removing these lines:

  const periods: [number, number][] = [
    [-100000, 1400],
    [1400, 1850],
    [1850, 1930],
    [1930, 2020],
  ];
  const [fromYear, toYear] =
    periods[Math.floor(Math.random() * periods.length)];

and these lines:

    if (candidate.year < fromYear || candidate.year > toYear) {
      return false;
    }

The requirement that a card be sufficiently distant from other cards as well as the requirement that a card be within a specific one of the above periods often led to no cards being found.

I'm not 100% sure what the code above was for—was it to weight the cards shown towards the present? If so, I'm sure I can find another way to do that, though honestly, I think it's a bit more fun not having those strictures in place.

[tom-james-watson]

I can't take a proper look at this right this second, but the motivation behind periods was to try and have a wider spread of card ages. The average year for the cards is fairly recent, so you ended up with very few older cards and too many 20th/19th century cards otherwise.

[benguaraldi]

@tom-james-watson Ah, got it! Okay, I've restored the periods code. The trick is to make the smallest period at least as big as the largest distance. I ended up adjusting the periods to be more than twice the largest distance, and now it seems to work fine.

[karenerobinson]

The original periods are nice. What if distance changed with the periods?

[benguaraldi]

@karenerobinson That seems overly complicated to me—is there a reason not to have a period from 1800-2020?

Also note that shortening the distance to keep it within the already-existing periods will have the opposite effect than what the periods were trying to achieve. (The periods were trying to spread the questions away from ones nearer to the present, but reducing the distance would pull the questions back towards the present.)

However, I just realized that my solution isn't sufficient. Once a user has reached a score of around 30, there will be no more valid cards in the periods. Let me think about how to fix that.

[karenerobinson]

@benguaraldi for scores over around 30, you could consider having a different filter block inside an if (candidates.length === 0) { }

(and then another if (candidates.length === 0) code segment to catch any errors that still result, after that)

[karenerobinson]

@benguaraldi

is there a reason not to have a period from 1800-2020?

Periods are entirely a matter of preference.

The aim to have a "good spread" could mean "some that feel modern" and "some that feel ancient," in which case it's nice to have one of the periods be more modern.

I don't have strong feelings either way - agree w your comment several days ago that "let's get it implemented."
My goal was to suggest a way to implement a minimum distance between years that keeps the original periods.

[benguaraldi]

@benguaraldi for scores over around 30, you could consider having a different filter block inside an if (candidates.length === 0) { }

@karenerobinson Excellent idea! I've implemented this—once there are 40 cards, it stops considering the periods when assigning new cards. (I used 40 instead of 30, as there is room for 44 cards 5 years apart from 1800-2020.)

I also changed the no new candidates error to use the full deck instead, which will at least let the player keep playing if the game gets into a state we didn't anticipate.

[benguaraldi]

@karenerobinson @tom-james-watson

I had another thought. Perhaps, instead of filtering the entire deck each time, we loop through the deck randomly and simply return the first card that matches our criteria.

Since there may be 0 cards that match our criteria, we give up after 10000 cards. (And by "give up" I mean, just return a random card.)

This should generally be quicker than the previous approach and should run in finite time in all cases.

The most recent code in this PR does that.

[benguaraldi]

@tom-james-watson I'm pretty sure that I've addressed the comments you made from the code review—could you take another look? Thanks!

[benguaraldi]

Thanks for the review, @tom-james-watson! I have made all of your requested changes. Let me know if anything looks awry.

[benguaraldi]

Hi @tom-james-watson! Just checking—how does this PR look now?

[benguaraldi]

@tom-james-watson?

[benguaraldi]

No problem, I totally understand! Thanks for merging it—it was fun to play the game with it implemented today!