Это нормально для функции, чтобы изменить параметр

17

У нас есть слой данных, который оборачивает Linq To SQL. В этом слое данных у нас есть этот метод (упрощенный)

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

При отправке изменений идентификатор отчета обновляется значением в базе данных, которое мы затем возвращаем.

С вызывающей стороны это выглядит так (упрощенно)

var report = new Report();
DataLayer.InsertReport(report);
// Do something with report.ID

Глядя на код, ID был установлен внутри функции InsertReport как своего рода побочный эффект, и затем мы игнорируем возвращаемое значение.

Мой вопрос, должен ли я положиться на побочный эффект и сделать что-то подобное вместо этого.

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

или мы должны предотвратить это

int InsertReport(Report report)
{
    var newReport = report.Clone();
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport.ID; 
}

может быть даже

Report InsertReport(Report report)
{
    var newReport = report.Clone();
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport; 
}

Этот вопрос был поднят, когда мы создали модульный тест и обнаружили, что на самом деле не совсем ясно, что свойство идентификатора параметров отчета обновляется и что для имитации поведения, связанного с побочными эффектами, было неправильно, если хотите, пахнет кодом.

Джон Петрак
источник
2
Для этого и нужна документация по API.

Ответы:

16

Да, все нормально, и довольно часто. Это может быть неочевидным, как вы обнаружили.

В общем, я склонен иметь методы типа постоянства, возвращающие обновленный экземпляр объекта. То есть:

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

Да, вы возвращаете тот же объект, что и переданный, но это делает API более понятным. Нет необходимости в клоне - если что-то вызовет путаницу, если, как в исходном коде, вызывающая сторона продолжает использовать переданный объект.

Другой вариант - использовать DTO

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

Таким образом, API очень очевиден, и вызывающая сторона не может случайно попытаться использовать переданный / модифицированный объект. В зависимости от того, что делает ваш код, это может быть немного болезненно.

Бен Хьюз
источник
Я бы не вернул объект, если бы Ира не понадобилась. Это выглядит как дополнительная обработка и использование памяти. Я иду в пустоту или исключение, если не в необходимости. Еще, я голосую за ваш образец DTO.
Независимый
Все эти ответы в значительной степени подводят итог наших собственных дискуссий. Это кажется вопросом без реального ответа. Ваше утверждение «Да, вы возвращаете тот же объект, что и переданный, но это делает API более понятным». в значительной степени ответил на наш вопрос.
Джон Петрак
4

IMO, это редкий случай, когда побочный эффект изменения желателен - поскольку у вашей сущности Report есть идентификатор, который, как можно предположить, уже имеет отношение к DTO, и, возможно, существует обязанность ORM гарантировать, что отчет в памяти сущность синхронизируется с объектом представления базы данных.

StuartLC
источник
6
+1 - после вставки сущности в БД вы ожидаете, что у нее будет идентификатор. Было бы более удивительно, если бы сущность вернулась без нее.
MattDavey
2

Проблема в том, что без документации вообще не ясно, что делает метод и особенно почему он возвращает целое число.

Самым простым решением было бы использовать другое имя для вашего метода. Что-то вроде:

int GenerateIdAndInsert(Report report)

Тем не менее, это не является достаточно однозначным: если, как и в C #, экземпляр reportобъекта передается по ссылке, было бы трудно узнать, reportбыл ли изменен исходный объект, или если метод клонировал его и изменил только клон. Если вы решили изменить исходный объект, было бы лучше назвать метод:

void ChangeIdAndInsert(Report report)

Более сложное (и, возможно, менее оптимальное) решение заключается в серьезном рефакторинге кода. Как насчет:

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();
}
Арсений Мурзенко
источник
2

Обычно программисты ожидают, что только методы экземпляра объекта могут изменить его состояние. Другими словами, я не удивляюсь, если report.insert()изменится идентификатор отчета, и это легко проверить. Нелегко задаться вопросом для каждого метода во всем приложении, изменяет ли он идентификатор отчета или нет.

Я также утверждаю, что, возможно, IDвообще не должен принадлежать Report. Поскольку он не содержит действительного идентификатора так долго, у вас действительно есть два разных объекта до и после вставки, с разным поведением. Объект «before» может быть вставлен, но его нельзя извлечь, обновить или удалить. Объект «после» - полная противоположность. У одного есть идентификатор, а у другого нет. Способ их отображения может быть различным. Списки, в которых они появляются, могут быть разными. Связанные пользовательские разрешения могут быть разными. Они оба являются «отчетами» в английском смысле этого слова, но они очень разные.

С другой стороны, ваш код может быть достаточно простым, чтобы одного объекта было достаточно, но это то, что нужно учитывать, если ваш код приправлен if (validId) {...} else {...}.

Карл Билефельдт
источник
0

Нет, это не хорошо! Подходит для процедуры изменения параметра только в процедурных языках, где нет другого пути; в языках ООП вызывайте метод изменения объекта, в данном случае - отчета (что-то вроде report.generateNewId ()).

В этом случае ваш метод делает 2 вещи, следовательно, нарушает SRP: он вставляет запись в БД и генерирует новый идентификатор. Вызывающая сторона не может знать, что ваш метод также генерирует новый идентификатор, так как он просто называется insertRecord ().

m3th0dman
источник
3
Эмм ... что если вызвать db.Reports.InsertOnSubmit(report)метод change объекта?
Стивен С
Ничего страшного ... этого следует избегать, но в этом случае LINQ to SQL выполняет модификацию параметра, поэтому OP не может избежать этого без эффекта клона с прыжковым обручем (что является его собственным нарушением SRP).
Теластин
@ StephhenC Я говорил, что вы должны вызвать этот метод для объекта отчета; в этом случае нет смысла передавать тот же объект в качестве параметра.
m3th0dman
@Telastyn Я говорил в общем случае; передовой опыт не может соблюдаться на 100%. В его конкретном случае никто не может определить, как лучше всего из 5 строк кода ...
m3th0dman