Il problema qui è che un User può effettivamente contenere due cose diverse:

1. Un'entità utente completa, che può essere passata al tuo archivio dati.

2. Il set di elementi di dati richiesti dal chiamante per iniziare il processo di creazione di un'entità utente. Il sistema deve aggiungere un nome utente e una password prima che sia veramente un utente valido come al punto # 1.

Questo include una sfumatura non documentata al modello dell'oggetto che non è affatto espressa nel tuo sistema di tipi. Devi solo "conoscerlo" come sviluppatore. Non è eccezionale, e porta a schemi di codice strani come quello che stai incontrando.

Suggerirei che hai bisogno di due entità, ad es. una classe User e una classe EnrollRequest . Quest'ultimo può contenere tutto ciò che è necessario sapere per creare un utente. Sembrerebbe la tua classe utente, ma senza il nome utente e la password. Quindi potresti fare questo:

public User InsertUser(EnrollRequest request) {
    var userName = GenerateUserName();
    var password = GeneratePassword();

    //You might want to replace this with a factory call, but "new" works here as an example
    var newUser = new User
    (
        request.Name, 
        request.Email, 
        userName, 
        password
    );
    context.Users.Add(user);
    return newUser;
}

Il chiamante inizia con le sole informazioni sulla registrazione e recupera l'utente completato dopo che è stato inserito. In questo modo eviti di mutare le classi e hai anche una distinzione di tipo sicuro tra un utente che è inserito e uno che non lo è.

Gli effetti collaterali sono ok se non vengono inaspettati. Quindi non c'è nulla di sbagliato in generale quando un repository ha un metodo che accetta un utente e modifica lo stato interno dell'utente. Ma IMHO un nome di metodo come InsertUser non comunica chiaramente questo , e questo è ciò che lo rende incline agli errori. Quando si utilizza il repository, mi aspetto una chiamata come

 repo.InsertUser(user);

per cambiare lo stato interno dei repository, non lo stato dell'oggetto utente. Questo problema esiste in entrambi delle tue implementazioni, in che modo InsertUser lo fa internamente è completamente irrilevante.

Per risolvere questo problema, puoi

• inizializzazione separata dall'inserimento (quindi il chiamante di InsertUser deve fornire un oggetto User completamente inizializzato, oppure

• costruisce l'inizializzazione nel processo di costruzione dell'oggetto utente (come suggerito da alcune delle altre risposte), o

• prova semplicemente a trovare un nome migliore per il metodo che esprima più chiaramente ciò che fa.

Quindi scegli un nome di metodo come PrepareAndInsertUser , OrchestrateUserInsertion , InsertUserWithNewNameAndPassword o qualsiasi altra cosa tu preferisca per rendere più chiaro l'effetto collaterale.

Ovviamente, un nome di metodo così lungo indica che il metodo sta forse facendo "troppo" (violazione SRP), ma a volte non si vuole o non si può risolvere facilmente, e questa è la soluzione meno invasiva e pragmatica.

Sto osservando le due opzioni che hai scelto, e devo dire che preferisco di gran lunga il vecchio metodo piuttosto che il nuovo metodo proposto. Ci sono alcune ragioni per farlo anche se fondamentalmente fanno la stessa cosa.

In entrambi i casi devi impostare user.UserName e user.Password . Ho delle riserve sull'articolo della password, ma quelle riserve non sono pertinenti all'argomento in questione.

Implicazioni di modifica degli oggetti di riferimento

• Rende più difficile la programmazione concorrente - ma non tutte le applicazioni sono multithreading
• Quelle modifiche possono essere sorprendenti, in particolare se nulla sul metodo suggerisce che accada
• Quelle sorprese possono rendere più difficile la manutenzione

Vecchio metodo rispetto a nuovo metodo

Il vecchio metodo ha reso le cose più facili da testare:

• GenerateUserName() è testabile indipendentemente. Puoi scrivere test su quel metodo e assicurarti che i nomi siano generati correttamente
• Se il nome richiede informazioni dall'oggetto utente, puoi cambiare la firma in GenerateUserName(User user) e mantenere tale testabilità

Il nuovo metodo nasconde le mutazioni:

• Non sai che l'oggetto User sta cambiando fino a diventare 2 strati profondi
• Le modifiche all'oggetto User sono più sorprendenti in tal caso
• SetUserName() fa di più che impostare un nome utente. Non è vero nella pubblicità che rende più difficile per i nuovi sviluppatori scoprire come funzionano le cose nella tua applicazione

Sono pienamente d'accordo con la risposta di John Wu. Il suo suggerimento è buono. Ma manca leggermente la tua domanda diretta.

Basically, is the practice of setting a property's value from another method a bad practice?

Non inerentemente.

Non puoi prenderlo troppo lontano, poiché ti imbatterai in comportamenti imprevisti. Per esempio. PrintName(myPerson) non dovrebbe cambiare l'oggetto persona, poiché il metodo implica che è interessato solo a leggere i valori esistenti. Ma questo è un argomento diverso rispetto al tuo caso, dal momento che SetUsername(user) implica strongmente che sta per impostare i valori.

Questo è in realtà un approccio che uso spesso per i test di unità / integrazione, in cui creo un metodo specifico per modificare un oggetto al fine di impostare i suoi valori su una particolare situazione che voglio testare.

Ad esempio:

var myContract = CreateEmptyContract();

ArrrangeContractDeletedStatus(myContract);

Mi aspetto esplicitamente che il metodo ArrrangeContractDeletedStatus cambi lo stato dell'oggetto myContract .

Il vantaggio principale è che il metodo consente di testare la cancellazione del contratto con diversi contratti iniziali; per esempio. un contratto con una cronologia di stato lunga o con una cronologia di stato precedente, un contratto con una cronologia di stato intenzionalmente errata, un contratto a cui l'utente di prova non è autorizzato a eliminare.

Se avessi fuso CreateEmptyContract e ArrrangeContractDeletedStatus in un unico metodo; Dovrei creare più varianti di questo metodo per ogni contratto diverso che vorrei testare in uno stato eliminato.

E mentre potrei fare qualcosa di simile:

myContract = ArrrangeContractDeletedStatus(myContract);

Questo è ridondante (dato che sto cambiando comunque l'oggetto), oppure ora mi sto costringendo a creare un clone profondo dell'oggetto myContract ; che è eccessivamente difficile se si vuole coprire ogni caso (immaginiamo se voglio avere proprietà di navigazione di diversi livelli. Devo clonarli tutti? Solo l'entità di livello superiore? ... Tante domande, così tante aspettative implicite )

Cambiare l'oggetto è il modo più semplice per ottenere ciò che voglio senza dover fare più lavoro solo per evitare di non cambiare l'oggetto in linea di principio.

Quindi la risposta diretta alla tua domanda è che non è una pratica intrinsecamente cattiva, se non offuschi che il metodo è suscettibile di cambiare l'oggetto passato. Per la tua situazione attuale, ciò è reso chiaramente chiaro attraverso il nome del metodo.

Trovo il vecchio e il nuovo problematico.

Vecchio modo:

1) InsertUser(User user)

Durante la lettura del nome di questo metodo che appare nel mio IDE, la prima cosa che mi viene in mente è

Where is the user inserted into?

Il nome del metodo dovrebbe leggere AddUserToContext . Almeno, questo è, ciò che il metodo fa alla fine.

2) InsertUser(User user)

Ciò viola chiaramente il principio della sorpresa minima. Dì, ho fatto il mio lavoro finora e ho creato una nuova istanza di User e gli ho dato un name e impostato un password , sarei sorpreso dalla sorpresa:

a) questo non solo inserisce un utente in qualcosa

b) fa anche sovvertire la mia intenzione di inserire l'utente così com'è; il nome e la password sono stati sovrascritti.

c) indica che è anche una violazione del principio di responsabilità singola.

Nuovo modo:

1) InsertUser(User user)

Ancora violazione di SRP e principio della sorpresa minima

2) SetUsername(User user)

Domanda

Set username? To what?

Migliore: SetRandomName o qualcosa che riflette l'intenzione.

3) SetPassword(User user)

Domanda

Set password? To what?

Migliore: SetRandomPassword o qualcosa che riflette l'intenzione.

Suggerimento:

Quello che preferirei leggere sarebbe qualcosa di simile a:

public User GenerateRandomUser(UserFactory Uf){
    User u = Uf.GetUser();
    u.Name = GenerateRandomUserName();
    u.Password = GenerateRandomUserPassword();
    return u;
}

...

public void AddUserToContext(User u){
    this.context.Users.Add(u);
}

Per quanto riguarda la tua domanda iniziale:

Is modifying an object passed by reference a bad practice?

No. È più una questione di gusti.