Skip to content

Conversation

@br1n0
Copy link

@br1n0 br1n0 commented Nov 19, 2015

hi, I added a function to extract omocodia number from cf ,
and combatibile declarations with node.js and browser,
just declared as functions and exported as modules when modules exists ( in node as example ), this way is possible use it in angular or inside the browser in general, without any changes.
I also added a kiss testing, the previus declaration require chai.

@br1n0
Copy link
Author

br1n0 commented Nov 19, 2015

Passo all'italiano, che mi viene piu semplice :)
ho aggiunto anche una direttiva per utilizzarlo in angular, le aggiunte non cambiano il comportamento della funzione dichiarata da te, ma aggiungono una versione che non ritorna le eccezioni, io trovo scomodo gestire il flusso di esecuzione con le eccezioni, spece con il codice quando il gioco si fa duro ed il codice complesso.
Ciao
B

@lambrojos
Copy link
Member

Ciao allora direi che la funzionalità che hai aggiunto va più che bene :)
Ci sono però delle questioni di forma:

  • cosa si guadagna a togliere mocha e chai? io non vedo problemi ad avere dipendenze esterne se aiutano ad avere test leggibili e manutenibili. Posso capire che chai e le asserzioni BDD in generale possano non piacere ma senza gli it() e i describe() diventa veramente dura, più che altro perchè non si può usare un testrunner.
    Inoltre sarebbero soltando una dev dependency, installandalo con --no-dev non c'è overhead ne dipendenze.
    ( mi sono reso conto che non abbiamo messo mocha e chai nelle dev dependencies errore nostro). Scusami ma questo ci impedisce il merge.

  • buona idea l'integrazione con angular però io la sposterei in una cartella examples/angular. Oppure puoi creare un repo per la direttiva ed aggiungere questo come dipendenza, in generale questo vorrebbe un modulo framework agnostic.

  • non sono d'accordo sul fatto che il throw non vada bene in situazioni complesse. Per esempio, tutte le librerie di promise (inclusa la versione di q usata da angular) funzionano in modo molto simile al try/catch dato che hai una catena di then() e un catch() per beccare eventuali errori che avvengono durante tutto un pezzo di elaborazione.
    Inoltre ritornando semplicemente true o false si perde la motivazione originale dell'errore, dato che la validazione è complessa. Effettivamente dovremmo aggiungere test in cui sono dimostrati i vari tipi di errore.
    Poi:

    if( r=== false )
    return true;
    throw new Error( r );

Così facendo si ottiene un Error in cui error.name è 'false'.. non è molto utile

  • non sono super sicuro di voler rendere opzionale l'aderenza a commonJS, ormai browserify/webpack/jspm sono ovunque.. o sbaglio? Il frontend cosa ne pensa? @ilgianfra @dbertella @ibbatta
  • occhio all'indentazione :)

@br1n0
Copy link
Author

br1n0 commented Nov 20, 2015

Ciao Giovanni,
Grazie per la fulminea risposta!
ma veniamo alle questioni di forma:

il test come era non mi funzionava, ho istallato chai e lanciato con node ./codice_fiscale_testjs ma mi ritornava errore, non ho approfondito piu di tanto, probabilmente mi mancava qualcosa, ma visto che difatto il test controlla semplicmente un identita, mi veniva fascile scriverlo in una funzione, cosi quelli che non gradiscono dipendenze esterne, possono vivere felici e contenti. Io sono un vecchio utente linux, potresti indicarmi come fare ad installarlo con --no-dev?
se preferisci elimino il mio test kiss, non e' molto importante.

mi piace l'idea della cartella e del modulo framework agnostic.
vuoi che te la sposto?

Salvo errori checkCf si comporta come la hai definito tu,
modificare il codice esitente a meno di errori madornali e' sempre da evitare e trovo scortesi gli sviluppatori che pasticciano le api guastanto la compatibilita al passato .

dato che isInvalidCF ritorna la stringa del messaggio di errore atteso

function checkCf(codiceFiscale)
{
var r= isInvalidCF(codiceFiscale);

if( r=== false ) 
    return true;

throw new Error( r );

}

percui checkCf ritorna true, se e solo se isInvalidCF ha ritornato false ovvero non ha tornato errori, nel caso invece che codiceFiscale presenti errori, r contiene la descrizione dell'errore lancia l'eccezione con la descrizione.
L'ho scritta col false per essere il piu specifico possivbile, una scrittura piu leggibilile e':

function checkCf(codiceFiscale)
{
var errore= isInvalidCF(codiceFiscale);
if( !errore )
return true;
throw new Error( errore );
}

circa il throw catch, a te mi sembra ti piacciano perche le usi insiee con i .then di $q in chain, a me non piace perche quando voglio ho una serie i chiamate il cui esito e' legato tra loro, voglio poter catturare gli errori singolarmente e cosi mi costringe a gestire molti blocchi try e catch.

Insomma non mi piacciono i blocchi try e catch per gestire il flusso di esecuzione,
(forse sono stato esposto troppo al c da bambino)
immagina che dato un cf lo debba validare ed inviare i dati con una chiamata rest,
avrei il la validazione col suo blocco try e cath, e poi l'invio dei dati con il suo try e catch o metteresti tutto insieme, mettendo tutto insieme riduci i cambi di flusso strani alla goto, ma il codce del catch diventa fortemente accoppiato, non sarebbe bello avere una libreria che e' agnostica anche rispetto all'uso delle eccezioni?

In una funzione di tese concettualmente e' un po strano lanciare un eccezione, ti faccio un esempio per provare a spiegarmi:
se ho una funzione che ha il compito di testare che un numero sia pari, ti sembra
comodo che lanciasse un eccezione quando viene applicata ad un disparo?

per l'indentazione personale ti chiedo scusa, quella piu diffusa con le parentesi { perte sulla stessa linea popolare in java a me non piace, e preferisco quella del kernel con { su una nuova linea, ma sono gusti.
I miei sono un po particolari: a me piace lo spazio prima delle virgole invece che dopo, cosi a colpo d'occhio trovo le cose, e piace il codice con meno ;
Vuoi che te lo riformatto?

Ciao Bruno

@lambrojos
Copy link
Member

Ehilà
il test è fatto per girare con mocha https://mochajs.org/
quindi

(sudo) npm install -g mocha
mocha test.js

Ieri ho scritto molto di fretta quindi in realtà intendevo --production

(ti rimando alla documentazione di npm per i vari flag https://docs.npmjs.com/cli/install
quindi per farla breve

 npm install --production linkmesrl/codice_fiscale_validator

Non lo abbiamo ancora pubblicato su npm.org dato che come avrai notato manca un pò di documentazione!

Quindi per rispondere alla tua domanda, si mi piacerebbe tenere il test con chai.

Poi sI, sarebbe ottimo spostare l'esempio con angular :)

Invece per il coding style vedo di caricare un .eslintrc con le nostre linee guida, almeno abbiamo un modello da seguire

Per la questione try/catch, (se non ho capito male il tuo esempio), facendo un discorso che prescinda dalla validazione del codice fiscale, a me piace fare cose del genere:

    function validate(){
      checkCf(cf);
      qualcosaChePotrebbeSollevareErrori();
      ...
    }

    function asyncSave(toSave){
      // fai quello che devi fare e torna una promise
    }

    function errorHandler(error){
        //svolgi operazioni comuni a tutti gli errori
        logError(err);

        //gestisci caso per caso
        switch(err.name){
        }
    }

    Promise.try(validate(thing))
    .then(asyncSave)
    .then(anotherFunc)
    .catch(errorHandler)

In generale sono dell'idea che la gestione degli errori deve essere disaccoppiata dalla logica della funzione. Questo diventa evidente se prendiamo in considerazione il fatto che spesso la gestione degli errori prevede operazioni che prescindono dal tipo di errore (un esempio banale è il logging)

Avere tanti

     var result = validateCf(cf);
     if(result !== true){
         logError()
         //gestisci errore specifico
        return; 
     }

    var result2 = altroControllo(boh)
    if(result2 !=== true){
       logError();
       //gestici altro errore
       return [...]
    } 
  • aggiunge complessità in senso stretto alla funzione, per ogni if che metti raddoppi i path possibili di esecuzione
  • accoppia due responsabilità molto diverse (salvare un profilo utente e gestire gli errori)
  • rende il codice meno manutenibile: cosa succede se tipo decidessimo che oltre a loggare l'errore dobbiamo, che ne so, mostrare il testo dell'utente con una funzione showError()? Io vedo tre possibilità:
    • modificare tutti gli if? Non mi sembra tanto una buona idea, si avrebbe una duplicazione della logica che sul lungo periodo diventa ingestibile( sul principio DRY siamo d'accordo no?)
    • modificare logError() ed aggiungerci showError() in fondo? Mi sembra un buon modo per mischiare due responsabilità molto diverse (anche sul single responsiblity principle siamo d'accordo?)
    • si impacchetta logError() e showError() in un'altra funzione errorHandler() e si fattorizza? Buona idea, ma a questo punto si sta facendo la stessa cosa del try/catch, ma in modo più verboso :)

Quindi per concludere, il try/catch per me è semplicemente dello zucchero sintattico che facilita il disaccoppiamento della logica vs la gestione degli errori.

Poi diciamo che personalmente non amo molto le funzioni che ritornano dati di tipi diversi, alla fine è questo il motivo per cui ho messo il throw.

Riguardo alla tua domanda sui numeri pari e dispari:
Una funzione come quella del tuo esempio è un predicato ed è giusto che ritorni un booleano perchè esiste un solo motivo per cui un numero non è pari. Possono però esistere diversi motivi per cui un codice fiscale non è valido. Anche nella tua proposta la funzione non sarebbe un predicato perchè ritorna altro oltre che true/false, cambia solo il modo in cui viene riportato l'errore

In realtà si mi rendo conto che il nome isCodiceFiscale è sbagliato perchè effettivamente potrebbe farlo sembrare un predicato, sarebbe giusto chiamarla validateCodiceFiscale o qualcosa del genere.

Mentre scrivevo questo pippone infinito mi è venuto in mente che l'approccio giusto potrebbe essere quello che viene usato da libphonenumber di google che fa più o meno la nostra stessa cosa ma con i numeri di telefono. Per esempio la nostra interfaccia potrebbe essere:

var cf = parseCF(cf);
cf.isValid; //boolean
cf.errors: //Array<Error>|null
cf.omocodiaNumber: //String    
// oppure
cf.getOmocodiaNumber();//String

cosa ne dici?

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