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 #256

Closed idanstark42 closed 7 years ago

idanstark42 commented 7 years ago

Notice: like many of the other PRs, this must be merged before the automated broadcasting

idanstark42 commented 7 years ago

So after a talk with Martin, I'm going to perform a few changes to this PR:

  1. Re-integrating the sanitizing functions into the scores.json because they we're deleted without a reason
  2. Move all scores and rankings calculations to a shared code base which will be shared by the server and the client.
  3. Normally the server will run the calculation and save the files itself.
  4. When he can't reach the server (either the server is done, or the network is down), the client will save the changes in his $localStorage, and run them locally using the code. Then, if and when the the connection is restored, he will run the updates on the server and reload the new scores.json file from it, with his and all the other client's changes.

Note: the current way I thought of sharing the code, after a brief look online, is like described here: https://gist.github.com/sevcsik/9207267 I will put the file in a new folder called "shared", and reference it from the angular config.js file, and from the localserver.js file. Notes and opinions are more then welcome!

rikkertkoppes commented 7 years ago

I wouldn't do it like the gist. The problem there is that you have platform logic mixed with application logic.

You can write your services as just a js class, which ends up being the factory. And make it all plain is. Then in add a static $inject prop for the angular dependency injection system.

For angular, include the class and add a series of module.service(yourClass) For node, require the class and instantiate it with dependencies

rikkertkoppes commented 7 years ago

Here is the idea (forgive any typos as I type this on a tablet)

//surround with amd header for proper export
class SilverBullet {
    constructor(Bullet, Silver) {
        // define the class further
    }
}
//this could be a static prop when using a transpiler
SilverBullet.$inject = ['Bullet', 'Silver']

//in angular
angular.module('app.silverbullet', ['app.silver', 'app.bullet'])
    .factory('SilverBullet', SilverBullet);

//node
let SB = require('SilverBullet');
let instance = SB(require('Bullet'), require('Silver'));

This makes sure the upper part (the shared code) is simple and platform agnostic (apart from the static $inject prop which doesn't really hurt)

Furthermore, you have your imports all in one place (right where you do your angular module definition) Lastly, this is more testable, as you can just inject mocks into the constructor for testing

idanstark42 commented 7 years ago

So I looked at the code that I did here, and it's kind of a mess to resolve this to the new way it should be done, so I'm going to change the things we did here in a new PR that I will open today.