FirstLegoLeague / fllscoring

FLL Scoring app. See Demo:
http://firstlegoleague.github.io/fllscoring/
GNU General Public License v2.0
14 stars 19 forks source link

Referees 2018/scores sync #257

Open idanstark42 opened 7 years ago

idanstark42 commented 7 years ago

Hi all, So as you see I'm back with this branch, and that's because fixing it did took me allot shorter then creating a new one. I tried creating the shared code between the client and the server, but as I saw, there isn't enough code to share, just pieces of code. If I was to finish it, what you would see is a massive pile of dependency injections for it to be generic. So, I decided not to go there, and to simply fix the problem that was with the last request: the client couldn't operate on his own. So, After splitting the ng-scores into several services (and one factory) for SRP purposes, I added the $independence service, to give us the ability to save actions that couldn't be sent to the server, and redo them later on, in case the server goes back up.

poelstra commented 7 years ago

BTW, can I please ask to not 'just fix the code', but instead first update/write a test for the now-failing scenario, then fix the code to see if the test correctly catches it?

I know writing tests can seem daunting, but this part of the code was fairly well covered with tests, and many of my comments would have surfaced by running the tests, and/or writing a few of your own.

idanstark42 commented 7 years ago

I think I cleared everything you said. I want to see how it will run in the tests and then I'll try and fix those too. I did decide on not including an implementation for DNC and DSQ for the time being, seems as it's not used.

robvw commented 7 years ago

Martin Poelstra wrote:

Also, we decided that playing, but having a score of 0 would be 'better' than not playing at all, or being disqualified. So "DNC" (DidNotCompete) and "DSQ" (DiSQualified) should be 'less than' 0 (or any numeric score, actually, but I think it was decided that it shouldn't be possible for a team that played to have a negative score to keep it child-friendly :)).

Idan Stark wrote:

I did decide on not including an implementation for DNC and DSQ for the time being, seems as it's not used.

To the best of my knowledge, what we decided on was this:

I think the current solution is to use MIN_INT for DSQ and MIN_INT+1 for DNC. The display can then simply replace all negatives with zeros. I remember one case of a DNC on the first round, so when showing intermediate results it had to be handled somehow (in that case we went with "--"), but the problem went away when they did participate in the second round. Ideally this would be configurable (I can imagine other tournament organisers to prefer displaying a "0" for DNC), but I can imagine that gets a very low priority. What to do with DSQ is even trickier, but also even rarer (or possibly non-existent?).

TL;DR: My preference would be to support negative scores "behind the scenes" and only convert them to zero when displaying them; it just seems more fair. I understand DNC and DSQ are low priority, but if possible, it would be nice if design decisions are made in such a way they are easy to add back in.

Hope this helps, Rob

Edit: Attempted to improve the formatting; I hope it's less unreadable now...

poelstra commented 7 years ago

BTW, please excuse the terse wording. Again, you've done great work! Some comments may come over as harsh, but are definitely not intended that way, just written quickly :)