È una cattiva pratica usare i condizionali con funzioni che cambiano lo stato del programma?

È un po 'un giudizio, ma direi che se devi chiamare createFile() all'inizio di un metodo è leggermente meglio memorizzare il risultato in una variabile. Questo perché se qualcuno aggiorna il codice in un secondo momento e deve verificare lo stato di fileCreated , è comune copiare la condizione dal blocco di codice esistente. Se si utilizza direttamente il metodo, l'editor deve introdurre la variabile e aggiornare la condizione esistente. Non è davvero la fine del mondo, quindi non mi farei impiccare. Se l'ultima istruzione era return createFile() , non introdurrei una variabile.

In realtà, sebbene sia preferibile non dover controllare la stessa condizione più di una volta in un metodo, evitando che tutto sia ottimale. Ciò renderebbe il sopra inutile. Quindi non è davvero una cosa tagliata e secca. Dipende in genere da qual è il livello di abilità della squadra.

Ho riferito un caso diverso ma, anche se il metodo non modifica nulla ma può restituire risultati diversi a chiamate diverse, è spesso necessario acquisire localmente il risultato per la correttezza.

Per me, l'odore del codice principale è che un metodo createFile() restituisce un valore booleano che indica il successo. Questo mi ricorda gli stili di programmazione degli anni '70.

Un metodo denominato createFile() implica un contratto che crea il file e, se non può, il contratto non è soddisfatto e il metodo deve generare un'eccezione. Con la versione booleana, un utente leggermente incurante, abituato alle attuali pratiche di programmazione, vedrà quel nome e chiamerà il metodo, ignorando il valore di ritorno booleano e assumerà che il file sia stato creato.

Se desideri che il contratto consenta di non creare un file in determinate circostanze, devi indicarlo nel nome, ad es. come createFileIfPossible() .

In genere, il codice utente crea un file perché è necessario per procedere, quindi la gestione delle eccezioni predefinita è ciò che desideri, interrompendo ogni ulteriore calcolo fino a un punto in cui sai come continuare. Metti un fermo lì e registra l'eccezione lì. Quindi non ci sarà più bisogno del frammento di codice dalla tua domanda.

Utilizzare il valore di ritorno direttamente da una funzione, invece di salvarlo in una variabile, è perfettamente corretto, se ne hai solo bisogno una volta. In effetti, non introdurre artefatti superflui è una buona idea, in quanto quindi non puoi perdere traccia di loro, né nominarli male.

Ciò che non va bene è il nome delle funzioni. Non mi aspetto che restituisca un bool , ma un File o qualsiasi altra cosa e genera un'eccezione in caso di errore. Nelle lingue sostenendo che dovrebbe essere probabilmente un ctor.
In alternativa, rinominarlo tryOpenFile() sembra accettabile.
Un'altra stranezza è che non esiste un argomento per il nome del file.

Il codice deve essere facilmente leggibile. Se fai in modo che lo sviluppatore cerchi di scoprire cosa sta realmente accadendo nel tuo codice, c'è qualcosa di sbagliato.

Entrambi i casi sono leggibili per me, ma questo non significa che non possano essere migliorati. Capisco che il ritorno di "creare file" non sia così ovvio in termini di significato. La mia idea è creare un nuovo metodo (anziché una variabile) per rendere più naturale la lettura del codice.

Quindi, puoi migliorare un po 'con:

if (!isFileCreated(createFile())
    // log: we cannot create file

Ignora le chiamate effettive per un secondo, perché sono un dettaglio di implementazione e devono essere gestite e tali. Il tuo primo compito è: O creare un file e impostare un booleano "fileCreated" su true, o provare a creare un file ma fallire, ripulire tutto come se non si fosse mai tentato di creare un file e impostare un valore booleano "fileCreated" su false.

La tua seconda attività è: se il file non è stato creato, registra il fatto.

Ora scrivi il codice per la seconda attività. Dovresti essere in grado di farlo senza aver scritto il codice per la prima attività. Di conseguenza, non puoi avere una condizione come "if (createFile () == false). Il tuo primo esempio di codice ha dato il via alla creazione del file e al test. Era solo un piccolo pezzetto di codice, quindi te ne vai via , ma diventerà brutto nei casi più grandi.