Va bene per una funzione modificare un parametro

Sì, è OK e abbastanza comune. Tuttavia, può essere non ovvio, come hai scoperto.

In generale, tendo ad avere metodi di tipo persistence che restituiscono l'istanza aggiornata dell'oggetto. Cioè:

Report InsertReport(Report report)
{        
    db.Reports.InsertOnSubmit(report);
    db.SubmitChanges();
    return report; 
}

Sì, restituisci lo stesso oggetto passato, ma rende l'API più chiara. Non c'è bisogno del clone - se qualcosa che possa causare confusione se, come nel codice originale, il chiamante continua a usare l'oggetto che ha passato.

Un'altra opzione è usare un DTO

Report InsertReport(ReportDTO dto)
{
    var newReport = Report.Create(dto);
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport; 
}

In questo modo l'API è molto ovvia e il chiamante non può tentare accidentalmente di utilizzare l'oggetto passato / modificato. A seconda di cosa sta facendo il tuo codice, può essere un po 'un problema però.

IMO questo è un caso raro in cui è auspicabile l'effetto collaterale del cambiamento - poiché l'entità Report ha un ID si potrebbe già presumere che abbia i dubbi di un DTO, e probabilmente c'è l'obbligo dell'ORM per garantire che l'entità di report in memoria sia sincronizzata con l'oggetto di rappresentazione del database.

Il problema è che senza la documentazione, non è chiaro affatto cosa stia facendo il metodo e soprattutto perché restituisce un intero.

La soluzione più semplice sarebbe usare un nome diverso per il tuo metodo. Qualcosa come:

int GenerateIdAndInsert(Report report)

Tuttavia, questo non è abbastanza chiaro: se, come in C #, l'istanza dell'oggetto report viene passata per riferimento, sarebbe difficile sapere se l'oggetto report originale è stato modificato, o se il metodo clonato e modificato solo il clone. Se scegli di modificare l'oggetto originale, sarebbe meglio dare un nome al metodo:

void ChangeIdAndInsert(Report report)

Una soluzione più complicata (e forse meno ottimale) consiste nel rifattorizzare pesantemente il codice. Che dire:

using (var transaction = new TransactionScope())
{
    var id = this.Data.GenerateReportId(); // We need to find an available ID...
    this.Data.AddReportWithId(id, report); // ... and use this ID to insert a report.
    transaction.Complete();
}

Di solito i programmatori si aspettano che solo i metodi di istanza di un oggetto possano cambiare il suo stato. In altre parole, non mi sorprende che report.insert() cambi l'id del report ed è facile da controllare. Non è facile chiedersi per ogni metodo nell'intera applicazione se cambia l'id del rapporto o no.

Vorrei anche sostenere che forse ID non dovrebbe nemmeno appartenere a Report . Poiché non contiene un ID valido per così tanto tempo, hai davvero due oggetti diversi prima e dopo l'inserimento, con un comportamento diverso. L'oggetto "prima" può essere inserito, ma non può essere recuperato, aggiornato o cancellato. L'oggetto "dopo" è l'esatto opposto. Uno ha un id e l'altro no. Il modo in cui vengono visualizzati potrebbe essere diverso. Le liste in cui appaiono potrebbero essere diverse. Le autorizzazioni utente associate possono essere diverse. Sono entrambi "rapporti" nel senso inglese del termine, ma sono molto diversi.

D'altro canto, il tuo codice potrebbe essere abbastanza semplice da rendere sufficiente un oggetto, ma è qualcosa da considerare se il tuo codice è costellato di if (validId) {...} else {...} .

No, non va bene! È adatto per una procedura per modificare un parametro solo in linguaggi procedurali, dove non esiste altro modo; nei linguaggi OOP chiama il metodo di modifica sull'oggetto, in questo caso sul report (qualcosa come report.generateNewId ()).

In questo caso il tuo metodo fa 2 cose, quindi rompe SRP: inserisce un record nel db e genera un nuovo id. Il chiamante non è in grado di sapere che il tuo metodo genera anche un nuovo id poiché viene semplicemente chiamato insertRecord ().