Che cosa dici in una revisione del codice quando l'altra persona ha creato una soluzione complicata? [chiuso]

Mentalità / atteggiamento

• Fornisci l'esempio
• ammonire in privato (one-to-one, al di fuori della revisione del codice)
• Incoraggia una mentalità Keep-it-simple tra i membri del team

Gestione del team

• Dedicare più tempo alle specifiche di un oggetto di lavoro (come architettura, struttura dell'algoritmo, wireframe dell'interfaccia utente, ecc.)
• Incoraggiare i membri del team a chiedere chiarimenti sulla portata di un oggetto di lavoro
• Incoraggiare i membri del team a discutere i modi per implementare un oggetto di lavoro
• Esegui stime ragionevoli per ciascun elemento di lavoro prima di iniziare e fai il massimo sforzo per soddisfarle
• Monitora il "miglioramento" dei membri del team.
  • Dopo essere stato ammonito o mostrato il modo giusto di fare le cose, verifica se il membro del team migliora.

Livello di abilità

• Assegna un po 'di tempo per sessioni di programmazione della coppia o sessioni di formazione individuale per sfruttare al meglio gli strumenti di sviluppo (refactoring, revisione del codice)

Gestione del progetto (rischio)

• Esegui la revisione del codice più spesso, in modo asincrono (Nota)
  • Nota su "asincronicamente"
    • Il revisore del codice dovrebbe ricevere notifiche / inviti per rivedere le modifiche non appena viene eseguito il commit
    • Il revisore del codice dovrebbe avere la possibilità di rivedere il codice prima di qualsiasi riunione con lo sviluppatore.
    • Se è necessario chiarire lo sviluppatore, farlo in modo informale su IM / email senza esprimere un parere negativo

What do you say in a code review when the other person built an over complicated solution?

Dici: "hai costruito una soluzione eccessivamente complicata."

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is "well it's already done."

Se è troppo tardi per cambiare qualcosa, perché stai facendo una revisione del codice?

"È già fatto" non è una risposta soddisfacente. Fatto significa testato e funzionante. Ogni codice extra che non sta facendo nulla di utile dovrebbe essere mantenuto nel modo corretto (cancellato).

Assegnagli di nuovo questo compito chiedendo di refactoring e ottimizzare la sua soluzione. Se non lo fa, assegnagli un programmatore di coppia e spera che imparerà qualcosa dal collega.

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is "well it's already done".

Questa non è una risposta accettabile:

• Se veramente è troppo tardi per cambiare, allora la revisione del codice sarà in gran parte una perdita di tempo e la gestione deve saperlo.

• Se questo è davvero un modo per dire "Non voglio cambiare", allora devi prendere la posizione che la complessità extra è BAD per il codebase A CAUSA dei problemi / costi che dovrà sostenere dopo. E riducendo il potenziale per problemi futuri, la vera ragione per cui stai facendo la revisione del codice, in primo luogo.

E ...

... the solution wasn't fully functional ...

Questo è probabilmente un risultato diretto della complessità non necessaria. Il programmatore lo ha reso così complesso che non lo comprende più completamente e / o ha perso tempo a implementare la sua complessità piuttosto che i punti di funzione. Vale la pena ricordare al programmatore che tagliare la complessità potrebbe portarlo a un programma di lavoro più veloce.

Ora sembra che non hai il potere (o forse la sicurezza) di "spingere indietro" su questo. Ma anche così, vale la pena fare un po 'di rumore su questo (senza personalizzarlo) nella speranza che il codificatore offendente farà un lavoro migliore ... la prossima volta.

What is an appropriate response to this attitude?

In definitiva, portalo all'attenzione della direzione ... a meno che tu non abbia il potere di aggiustarlo da solo. (Naturalmente, questo non ti renderà popolare.)

Avevi ragione, avevano torto:

• broken YAGNI principio
• broken KISS principio
• il codice è stato completamente testato? Se no, allora non è fatto

What is an appropriate response to this attitude?

Esegui la corretta revisione del codice. Se si rifiutano di implementare le modifiche suggerite senza una ragione, quindi smettere di sprecare il tuo tempo una revisione del codice. Puoi anche inoltrare il problema al loro capo .

Un'azione condotta dal nostro team, che ha migliorato notevolmente la situazione in questi casi, è stata la transizione a molti changeset più piccoli .

Invece di lavorare su una sola attività per un giorno o più e poi avere una (grande) revisione del codice, cerchiamo di effettuare il check-in molto più spesso (fino a 10 volte al giorno). Naturalmente questo ha anche alcuni inconvenienti, ad es. il revisore deve essere molto reattivo, il che diminuisce il proprio output (a causa di frequenti interruzioni).

Il vantaggio è che i problemi vengono rilevati e possono essere risolti in anticipo, prima che venga eseguita una grande quantità di lavoro nel modo sbagliato.

Dovresti concentrarti sulla causa principale del problema:

1. La formazione dei programmatori si concentra sull'aumento della complessità data ai programmatori. La capacità di farlo è stata testata dalla scuola. Così molti programmatori penseranno che se implementano una soluzione semplice, non hanno fatto il loro lavoro correttamente.
2. Se il programmatore segue lo stesso modello che ha fatto centinaia di volte mentre era all'università, è proprio come stanno pensando i programmatori: più complessità è più difficile e quindi migliore.
3. Quindi per risolvere questo problema devi mantenere una rigorosa separazione dei requisiti della tua azienda rispetto alla complessità rispetto a quanto normalmente richiesto nella formazione del programmatore. Un buon piano è una regola come "il livello di complessità più alto dovrebbe essere riservato solo alle attività progettate per migliorare le tue abilità - e non dovrebbe essere usato nel codice di produzione".
4. Diventerà una sorpresa per molti programmatori che siano non autorizzati a fare i loro disegni più pazzi nell'importante ambiente del codice di produzione. Basta riservare del tempo ai programmatori per fare progetti sperimentali e quindi mantenere tutta la complessità in quel lato del recinto.

(nella revisione del codice è già troppo tardi per cambiarlo)

Non conosco nulla che funzioni dopo che il codice è stato scritto.

Prima che il codice sia scritto, le persone possono discutere modi alternativi per farlo. La chiave sta contribuendo le idee l'un l'altro, quindi si spera che ne venga scelto uno ragionevole.

C'è un altro approccio che funziona con gli appaltatori - contratti a prezzo fisso. Più semplice è la soluzione, più $$ il programmatore riesce a mantenere.

Non puoi sistemare il mondo.

Non puoi nemmeno sistemare tutto il codice sul tuo progetto. Probabilmente non puoi correggere le pratiche di sviluppo sul tuo progetto, almeno non questo mese.

Purtroppo, ciò che stai sperimentando nella revisione del codice è fin troppo comune. Ho lavorato in un paio di organizzazioni in cui ho trovato spesso la possibilità di rivedere 100 righe di codice che avrebbero potuto essere scritte in dieci, e ho ottenuto la stessa risposta che hai fatto: "È già scritto e testato" o "Stiamo cercando bug, non una riprogettazione. "

È un dato di fatto che alcuni dei tuoi colleghi non possono programmare al meglio. Alcuni di loro potrebbero essere piuttosto cattivi. Non preoccuparti di questo. Un paio di classi con cattive implementazioni non faranno fallire il progetto. Invece, concentrati sulle parti del loro lavoro che influenzeranno gli altri. I test unitari sono adeguati (se li hai)? L'interfaccia è utilizzabile? È documentato?

Se l'interfaccia del codice errato è ok, non preoccuparti di ciò finché non lo devi mantenere, quindi riscrivilo. se qualcuno si lamenta, solo chiamalo refactoring. Se si lamentano ancora, cerca una posizione in un'organizzazione più sofisticata.

Ci dovrebbe essere una politica standard nel progetto che controlli le procedure di verifica della qualità consegnabile e gli strumenti utilizzati.

Le persone dovrebbero sapere cosa dovrebbero fare e quali strumenti sono accettati per l'uso in questo progetto.

Se non lo hai ancora fatto, organizza i tuoi pensieri e fallo.

La revisione del codice dovrebbe avere un elenco di controllo di articoli standard. Se si ottiene "è già fatto" e non lo è, personalmente, non vorrei essere responsabile per il lavoro di questo sviluppatore come project manager o sviluppatore senior. Questo atteggiamento non deve essere tollerato. Riesco a capire come si fa a fare qualcosa o anche ogni cosa, ma una volta accettata una soluzione, la menzogna non dovrebbe essere tollerata e dovrebbe essere chiaramente indicata.

Il tuo negozio deve applicare alcune metodologie di progettazione.

• I requisiti devono essere definiti chiaramente.
• Devi sviluppare casi d'uso che supportano i requirments.
• Devi specificare le funzioni necessarie per implementare i casi d'uso.
• Devi specificare i requisiti non funzionali (tempi di risposta, disponibilità ecc.)
• È necessario un RTM (Requirements Tracabilty Matrix) per mappare ogni funzione del sistema su un caso d'uso e un requisito reale.
• Elimina qualsiasi funzione che non supporta un requisito reale.
• Infine nella revisione del codice contrassegna qualsiasi codice che non implementa o supporta direttamente le funzioni definite.

Probabilmente non è troppo complicato perché in seguito la maggior parte delle persone si sente male. Presumo che, quando questo accade già, sia stato scritto un sacco di codice senza dirne una parola. (Perché è così? Perché quella persona ha abbastanza autorità quindi il suo codice non deve essere rivisto in realtà?)

Altrimenti suppongo di rendere la revisione del codice meno formale ma più frequente. E prima di scrivere grandi moduli forse dovresti discutere rapidamente quale approccio adottare.

Dire "questo è troppo complicato" non ti porta da nessuna parte.

È spiacevole, ma le recensioni di codice sono, molte volte, più per il futuro che per il presente. Soprattutto in un ambiente aziendale / aziendale, il codice fornito è sempre più prezioso del codice non mappato.

Questo, ovviamente, dipende da quando la revisione del codice è completata. Se fa parte del processo di sviluppo, allora potresti ottenere qualche beneficio in questo momento. Ma se la CR viene trattata come più post-mortem, allora è meglio precisare cosa potrebbe essere fatto meglio in futuro. Nel tuo caso (come altri hanno detto), sottolinea YAGNI e KISS in generale, e forse alcune delle aree specifiche in cui questi principi potrebbero essere applicati.

Che cosa significa troppo complicato? Fai una dichiarazione ambigua e otterrai una risposta ambigua / insoddisfacente in risposta. Ciò che è eccessivamente complicato per una persona è perfetto per un'altra.

Lo scopo di una revisione è di evidenziare problemi ed errori specifici, per non dire che non ti piace, che è ciò che l'affermazione "eccessivamente complessa" implica.

Se vedi un problema (troppo complicato), allora dì qualcosa di più concreto come:

• Non cambierebbe la parte da X a Y per semplificare il codice o renderlo più facile da capire?
• Non capisco cosa stai facendo qui nella parte X, penso che quello che stavi cercando di fare è questo. Presenta un modo più pulito di farlo.
• Come hai provato questo? Hai provato questo? Se è eccessivamente complicato, questo di solito porta a vuoti fissi. Chiedendo test, spesso la persona può semplificare autonomamente il proprio codice quando non è in grado di capire come testare il codice originale.
• Sembra che ci sia un errore qui, cambiare il codice per risolvere il problema.

Chiunque può segnalare problemi, soprattutto ambigui. C'è un sottoinsieme molto più piccolo che può presentare soluzioni. I commenti della tua recensione dovrebbero essere il più specifici possibile. Dire qualcosa è eccessivamente complesso non significa molto, può anche far pensare agli altri che tu sia incompetente per non essere in grado di capire il codice. Tieni presente che la maggior parte degli sviluppatori non ha la minima idea della differenza tra un progetto buono o cattivo.

A volte vale la pena che un gruppo si concentri su alcuni dei principi "Agili": possono aiutare un gruppo o un individuo che sembra essere un po 'fuori rotta.

Concentrarsi non deve significare una grande grande rielaborazione della tua squadra, ma dovresti sederti e discutere quali pratiche ti sono maggiormente importate in squadra. Suggerirei di discuterne almeno (e probabilmente molte altre):

• Fai la cosa più semplice che potrebbe funzionare?
• Non ne avrai bisogno (stai risolvendo problemi che non erano nelle specifiche)
• Scrivi il test prima della codifica (ti aiuta a focalizzare il tuo codice)
• Non ripeterti

Anche recensioni occasionali (settimanali?) di ciò che funziona, cosa no e cosa è ancora necessario può essere davvero utile ... Se non altro, perché non impegnarsi a un'ora alla settimana per discutere i valori e le pratiche del team?

Escalation, se hai un manager con una mentalità tecnica. Sembra un'abitudine che deve essere spezzata.

Se il codice non è costruito per specifiche, per definizione dovrebbe fallire la revisione del codice. Non capisco il concetto di "bene abbiamo fatto qualcosa che nessuno ha chiesto, e non funziona, quindi lo lasceremo lì invece di fare qualcosa che qualcuno ha chiesto per quello che funziona".

Questa è una cattiva abitudine per qualsiasi sviluppatore di entrare. Se lui / lei stava lavorando a una specifica di progettazione, quindi non la corrispondenza senza una buona ragione è un no no.

Una parola: agile

Di certo non risolve tutto. Ma regnando nelle tue iterazioni (1-2 settimane, per esempio), limitando il lavoro in corso e sfruttando la pianificazione / revisione dello sprint, dovresti evitare questi errori simili a cascate. Hai bisogno di una migliore visibilità su ciò che viene effettivamente fatto - mentre è in corso.

Per il normale sviluppo basato sul progetto, ti consigliamo di adottare un approccio Scrum . Per gli ambienti di sviluppo / integrazione continui e, soprattutto, se molti sviluppatori lavorano sullo stesso o su progetti correlati, considera l'incorporazione di elementi di Kanban . Un altro approccio efficace è quello di sfruttare la programmazione coppie , una pratica definita di Programmazione estrema .

La tua situazione non è affatto unica. E anche con team di piccole dimensioni, il processo può fare molto per evitare la situazione in cui ti trovi ora. Con una visibilità adeguata e un backlog ragionevolmente curato, domande come queste diventano decisioni di pianificazione dello sprint - salvandoti da gestione del debito tecnico .

Ciò che ho detto in passato è "questo codice è complesso e non sono sicuro di ciò che sta cercando di fare, è possibile semplificarlo o scriverlo più chiaramente?"

Sei al programmatore dopo aver cancellato / ripristinato il codice: "Oops, il tuo codice è andato. Per favore riscrivilo. Come hai già scritto una volta avrai bisogno di meno di venti minuti per fornire SOLO il codice richiesto dalle specifiche.

"La mia prossima recensione è tra 20 minuti.

"Buon giorno."

NON accettare argomenti!

Fatto, IMHO

Chris