[1824] Assign national presidency with equal shareholding#12390
[1824] Assign national presidency with equal shareholding#12390ollybh wants to merge 1 commit intotobymao:masterfrom
Conversation
The `tie_breaker` argument passed to the `set_national_president!` method is ordered by the ownership of the minors, ordered by the minor symbol. This means that the owner of SD1/KK1/UG1 will be the first player in the array. So when we sort to find the new president, we want to be looking for the lowest index in this array, not the highest. Fixes tobymao#12384.
|
|
||
| president_factors = president_candidates.to_h do |player, percent| | ||
| [[percent, tie_breaker.index(player), player == current_president ? 1 : 0], player] | ||
| [[percent, -1 * tie_breaker.index(player), player == current_president ? 1 : 0], player] |
There was a problem hiding this comment.
I think this will work but there another change that can be done and make it more similar to 1837 version.
Add after line 218:
tie_breaker = tie_breaker.reverse
And then the original 221 can be kept.
This is almost the same as 1837 except:
- @players is not a tie breaker in 1837 (The 1837 does not mention this, but I think theoretically two players without exchange shares can have a larger percentage than player with exchange shares.)
- tie_breaker.index will always be non-nil in 1824 so the "|| -1" is not needed.
This also means that ", player == current_president ? 1 : 0" is not needed as tie_breaker.index will always be different for different players.
In other words line 221 can be changed to:
[[percent, tie_breaker.index(player)], player]
There was a problem hiding this comment.
There is one more snag.
If tie_breaker contains (for 4 players - with player order 4 3 1 2) [1, 2, 3, 4, 3, 1, 2] and we do reverse, then
2 will have index 0, 1 will have index 1, 3 will have index 2, 4 will have index 3.
Which means 4 will be chosen first if equal percentage.
So tie_breaker need to be uniq-ified before reversed, as I believe any extra occurrence is removed.
tie_breaker.uniq would therefor return [1, 2, 3, 4]. The extra append of players is just to cover for the case where a player have bought shares in the Staatsbahn, without owning any pre-Staatsbahn.
So line added after 218 should be
tie_breaker = tie_breaker.uniq.reverse
perwestling
left a comment
There was a problem hiding this comment.
Thanks for looking into this.
I suggest a change, which I think simplifies the code a bit.
But I do believe your code works.
The
tie_breakerargument passed to theset_national_president!method is ordered by the ownership of the minors, ordered by the minor symbol. This means that the owner of SD1/KK1/UG1 will be the first player in the array. So when we sort to find the new president, we want to be looking for the lowest index in this array, not the highest.Fixes #12384.
There's a small chance this could break existing games, if a presidency is assigned to a different player, changing certificate counts. These games will need to be pinned or archived.
Before clicking "Create"
masterpinsorarchive_alpha_gameslabel if this change will break existing gamesdocker compose exec rack rubocop -adocker compose exec rack rake