Skip to content

Conversation

@Wesmania
Copy link

Signed-off-by: Igor Kotrasinski i.kotrasinsk@gmail.com

@Wesmania Wesmania force-pushed the fix-survived-stats-check branch from 82df28e to 7040bf8 Compare November 28, 2018 20:28
Also fixes broken tests.

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>
@Wesmania Wesmania force-pushed the fix-survived-stats-check branch from 3c8ea51 to f14909b Compare December 9, 2018 18:56
@Rackover
Copy link
Member

#262
#257

This problem has been addressed differently in the past.
As much as your method works, those work aswell. I am not sure of which is the best.
I would say that making get_army_result() returning the most reported result makes more sense than renaming the functions get_army_results and make it return multiple things. The former does fit more into the actual system.
We could do a compromise by making a function pick_most_reported that would do that operation, and use get_army_resulst to other extents (although for the moment, I see no use to such function returning a list of probably similar results).

What do you think ?

@Wesmania
Copy link
Author

I suppose the get_army_score function sets the precedent for that - although personally I'm a bit wary, since we also have functions that add new results to the result list, so in the same class we treat some data both as a list and as a magic singular result.
I don't think it matters which fix we choose, both are readable. If we ever revisit this code to refactor it properly, we'll probably move all the score-related methods to a separate class anyway.

@Rackover
Copy link
Member

After having seen DL/Crotalus's fix, I think i will go with that idea instead for the moment.
However I'd love to have you review it once it's done 🙂 Thanks for the help on this issue otherwise
Closing this PR to open another.

@Rackover Rackover closed this Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants