È accettabile assegnare una variabile di classe all'interno di un metodo?

Per rispondere alla tua domanda sull'argomento: dipende:)

In generale, e in particolare dal momento che l'hai taggato con codice pulito, preferiamo codice leggibile e gestibile.

Assegnazioni dirette come quella che hai dato spesso portano a un codice più efficiente, quindi, se questo è un fattore importante, potrebbe essere accettabile. Dovresti avere una ragione per una tale necessità di ottimizzazione. Possibili ragioni potrebbero essere lo sviluppo di un sistema o di benchmark in tempo reale che indichino chiaramente il rispettivo collo di bottiglia.

Senza una tale ragione, il codice più chiaro è più favorevole. Questo è in gran parte dovuto all'effetto di scrittura del codice una volta letture di 100 volte. Molte altre persone hanno bisogno di capire il codice nel corso della sua vita e io secondo il vostro sentimento che i metodi di effetti collaterali sono problematici sotto questo aspetto.

A seconda di quanto il tuo team valuta il codice pulito, le modifiche al codice richieste potrebbero essere più o meno drammatiche. Ecco alcune idee su come una tale situazione potrebbe essere affrontata portando a un codice più leggibile e più facile da manutenere. Spetta a te e al tuo team decidere quale sia più adatto al vostro progetto.

• Potresti dividere il metodo in due separati, uno per min e uno per max. Lato negativo: questo può portare alla duplicazione.
• Come suggerito nell'altra risposta, potresti voler introdurre un tipo Intervallo o Intervallo di qualche tipo per catturare min e max come una "cosa".
• Potresti essere in grado di calcolare questi valori una volta e memorizzarli come const / final, aprendo modi per iniettare questi valori nella creazione dell'oggetto.
• Potresti scoprire che l'intera archiviazione di questi valori nei campi è discutibile.

E se il tuo team sta andando molto lontano sulla qualità del codice, puoi ignorare completamente il corpo della tua funzione e affrontare i difetti già inerenti alla sua firma:

• Il nome del metodo suggerisce che c'è qualcosa da trovare, ma restituisce void . Quindi, il nome o il tipo restituito devono essere cambiati.
• La parte find del nome (proprio come search ) implica che potresti non trovare qualcosa. Come ci si confronta? Anche una volta che si ritorna f.ex. un tipo di intervallo si può ancora andare oltre e pensare a Forse / Optional-tipo di tipi - dopo tutto, qual è il minimo o il massimo di un array vuoto?

In sintesi, avrei una strong tendenza a rifiutare questo codice in una recensione. Tuttavia, a causa della natura molto locale del tuo esempio, è quasi impossibile offrirti una critica produttiva. Dopotutto, non dovresti rifiutare il codice in una recensione con "Non mi piace", ma nella migliore delle ipotesi dovresti offrire una variante decisamente migliore. Determinare ciò richiede però una maggiore conoscenza del design complessivo.

Per quanto riguarda le migliori pratiche: non credo che le migliori pratiche che funzionano su un campo così ristretto abbiano molto senso. Una volta che raggiungi un livello più alto con principi, come una singola responsabilità, dire non chiedere, ecc., Comportano forti implicazioni per questi pezzi più piccoli.

Senza sapere come sia il resto del codice, direi che questo è l'accattonaggio di un altro oggetto che rappresenta il massimo e il minimo.

Quando hai uno stato che viene in seguito, questo suggerisce un altro oggetto. Altrimenti, hai uno stato che non è inizializzato per un po 'di tempo, quindi in seguito viene inizializzato, quindi l'unico oggetto sta mescolando / confondendo lo stato con diverse durate. Tale conflazione può essere rimossa con un altro oggetto (istanza di classe o tupla), quindi ognuno di essi ha una durata di stato consistente.

In questo modo stai affrontando una serie di principi di progettazione. Single Responsibility, e inoltre, rende migliore l'astrazione. Quindi, ci sono due oggetti, l'originale con il suo scopo originale, ma può creare l'oggetto min / max e quindi l'oggetto min / max con il suo scopo e durata separati.

Un oggetto è un'astrazione - un raggruppamento di cose insieme aventi uno scopo comune e una durata . Se trovi una durata diversa, ciò suggerisce che il codice sta cambiando astrazioni, il che suggerisce un altro oggetto.

FWIW, gli stessi problemi, vite di stato conflating, si verificano nell'altra domanda citato da @ user2180613, che in quel caso sembra essere meglio risolta usando le variabili locali invece dei campi oggetto.

È anche preoccupante che questo sia un metodo di istanza di un oggetto già costruito che prende il doppio come parametro. Questo suggerisce che la classe in questione non è legata ad un particolare doppio array, e che questo metodo potrebbe essere chiamato più tardi con un diverso doppio array, cambiando il minimo e il massimo. Naturalmente, il metodo è privato, quindi forse questo va bene e solo i dettagli interni del costruttore; non possiamo sapere dallo snippet.

Tutto sommato, separare questi elementi in una tupla o un'altra istanza di classe per tornare sarebbe più pulito e non farebbe mendicare queste domande.

I metodi che cambiano lo stato degli oggetti sono assolutamente OK (a meno che l'oggetto non sia un DTO o un ValueObect).

(After returning, the calling code utilises these two values in various calculations; the calling code then calls other private methods that utilise these two values).

Questo è il problema ma nella direzione opposta.

Il "codice chiamante" funziona su queste proprietà è l'invidia per le funzionalità e questo codice appartiene molto probabilmente a questa classe.

In generale, Sì, va bene.

Ma ci sono argomenti contro di esso.

• Sicurezza thread: se il metodo viene chiamato da due thread contemporaneamente, hai una condizione di competizione?

• Variabili globali: è possibile ottenere risultati errati chiamando le funzioni che utilizzano min e max nell'ordine errato?

La programmazione funzionale sta ottenendo molta trazione su OOP in questi giorni perché evita problemi di stato interno.

Puoi restituire min e max da questa funzione (come una Tupla?) piuttosto che impostare lo stato se i valori sono usati immediatamente