Non sono molto entusiasta di nessuna delle due versioni. La seconda (singola grande espressione in return statement) rende difficile seguire la logica di controllo. Il fatto che l'autore abbia sentito il bisogno di commentare il codice per chiarire che cosa fa e che non si adatti a qualsiasi lunghezza ragionevole della linea sono indicatori che l'espressione è troppo complessa e dovrebbe essere spezzata.
Il primo esempio, al contrario, mi sembra troppo complicato. Ad esempio, come è
enum {
is_not_in_set = 0,
is_in_set = 1
};
in_set = FD_ISSET(socket_desc, &fdset);
if (in_set) {
// …
}
più leggibile del semplice utilizzo immediato del risultato della "chiamata di funzione" come condizionale?
if (FD_ISSET(socket_desc, &fdset)) {
// …
}
Ogni nome che introduci nello scope corrente aggiunge ulteriore carico cognitivo al lettore. Ciò potrebbe essere giustificato se aggiunge ulteriori informazioni, ma non vedo come l'uso di enum aiuti qui. È chiaro che una chiamata a FD_ISSET mi dice se il descrittore di file è nell'insieme o no. Non riesco a vedere come il enum sia utile qui. E l'altro enum ?
Allo stesso modo qui.
connection_to_target = (result == ECONNSUCCESS) || (result == ECONNREFUSED);
if (connection_to_target == 1) {
// …
}
C'è molta ridondanza qui. Dove viene dichiarato connection_to_target ? Qual è il suo tipo? Perché è rispetto a 1? Perché non è const ? Perché è lì?
Ancora, usando direttamente l'espressione
if ((result == ECONNSUCCESS) || (result == ECONNREFUSED)) {
// …
}
migliorerebbe notevolmente la leggibilità e ridurrebbe la complessità.
Nel terzo caso
sock_opt_read_result = getsockopt(socket_desc, SOL_SOCKET, SO_ERROR, &result, &result_len);
if (sock_opt_read_result >= 0) {
// …
}
potrebbe essere proposto un argomento per l'uso di una variabile denominata perché la chiamata alla funzione è molto lunga. Ma ancora una volta, ridurrebbe il carico cognitivo se la variabile fosse dichiarata const e inizializzata immediatamente. Inoltre, il nome sock_opt_read_result non è davvero utile. Il nome di una variabile dovrebbe indicare cosa significa, non da dove viene.
Infine, trovo il codice molto più leggibile quando si concentra sul "percorso del successo" e ne esce in caso di fallimento. Nei linguaggi di programmazione che hanno eccezioni, questo stile emerge in modo naturale. Ti aiuta a ridurre la profondità dei tuoi condizionali nidificati e rende più facile vedere a che punto abbiamo successo o fallimento.
Quindi, vorrei riscrivere il codice in qualche modo come questo.
if (!FD_ISSET(socket_desc, &fdset)) {
return Status_Failed;
}
if (getsockopt(socket_desc, SOL_SOCKET, SO_ERROR, &result, &result_len) < 0) {
return Status_Failed;
}
if ((result != ECONNSUCCESS) && (result != ECONNREFUSED)) {
return Status_Failed;
}
return Status_OK;
Se senti il bisogno di spiegare perché una certa condizione indica un fallimento, l'inizio del rispettivo blocco "allora" è un buon posto per inserire un breve commento.
L'ultimo condizionale nel codice suggerito sopra potrebbe essere controverso. L'ho scritto in questo modo per rimanere fedele al principio suggerito di mantenere dritto il "percorso del successo". Alternative come
return ((result == ECONNSUCCESS) || (result == ECONNREFUSED)) ? Status_OK : Status_Failed;
o
switch (result) {
case ECONNSUCCESS:
case ECONNREFUSED:
return Status_OK;
default:
return Status_Failed;
}
potrebbe anche essere una scelta ragionevole.
Sono abbastanza sicuro di poter essere d'accordo sul fatto che la sostituzione suggerita in questa risposta sia più leggibile rispetto a una delle alternative originali, ma questa risposta potrebbe non essere ancora basata sulla non opinione come speravate. Ho paura che non ci sia l'unico modo giusto per scrivere il tuo codice. Alla fine, penso che sia anche importante rispettare la personalità dei tuoi colleghi durante la revisione del codice. Se non è perfettamente chiaro in quale modo strutturare il codice è superiore, lascia che abbiano la sua versione e usa il tuo modo preferito per farlo nel codice che scrivi. Questi dettagli di implementazione sono locali per ciascuna funzione e non influiscono sulla capacità di manutenzione del software a un livello superiore. Se cambi la tua opinione in un secondo momento, puoi sempre rifattare in modo sicuro gli interni di qualsiasi funzione senza rischiare di rompere nulla.
A proposito, dai un'occhiata a Code Review se vuoi che altre persone discutano qualità di un pezzo del tuo codice.