Что возвращать, Empty collection или null?

Question

[Сергей Коновалов]

Собственно сам вопрос, что считается лучшей практикой, вернуть Enumerable.Empty() или null? И указывать возвращаемым типом List или IEnumerable?
Например есть метод возвращающий список адресов, какой вариант будет правильным?

public IEnumerable<Adresses> GetAdresses()
        {
            try
            {
                 return dbContext.Adresses.ToList();
            }
            catch (Exception)
            {
                return Enumerable.Empty<Adresses>();
            }
        }

или

public List<Adresses> GetAdresses()
        {
            try
            {
                 return dbContext.Adresses.ToList();
            }
            catch (Exception)
            {
                return null;//или что-нибудь вроде new List<Adresses>
            }
        }

И хотелось бы объяснений.

Comments

[Кирилл Таран]

К уже данному ответу стоит также добавить, что делать return из блока catch неправильно. Если коротко — нельзя использовать обработку исключений для неисключительных ситуаций.

[Сергей Коновалов]

Лучше throw ?

[Валерий Абакумов]

Кирилл Таран - согласен полностью. Да, Сергей Коновалов, в блоках catch лучше ставить throw. А еще лучше перед throw ставить логирование. Я бы добавил еще про разницу между

catch(Exception)
{
    throw;
}

и

catch(Exception ex)
{
    throw ex;
}

, но думаю, Вы это и так знаете.

[Кирилл Таран]

Сергей Коновалов не заметил вопроса. В данном случае лучше создать переменную до try-catch, и либо наполнять его данными в try, либо возвращать пустой, но в конце. В таком ключе:

public IEnumerable<Adresses> GetAdresses()
{
    var result = Enumerable.Empty<Adresses>()
    try
    {
        result = dbContext.Adresses.ToList();
    }
    catch (Exception ex)
    {
        //обработка настоящих ошибок
    }
    finally
    {
        return result;
    }
}

Попробуйте представить, что будет, если произойдёт реальная ошибка и, например, отвалится соединение с базой. Во многих сценариях возвращать какое-то значение уже не понадобится.

Answer 1

[Дмитрий Ковальский]

Лучше вернуть пустой список. Бизнес-логика как правило проверяет коллекцию на наличие хоть каких-то элементов(Count/Length>0) или по признаку (Where().Count/Length>0). Возврат NULL предварит эти проверки доп. проверкой на NULL. Лично вы из каких соображений хотите вернуть NULL?
UPD: Есть альтернатива - постоянно ловить NullReferenceException, но с тем же успехом можете использовать оператор goto.

Comments

[Сергей Коновалов]

Пробую силы в асп, беру из базы список адресов, передаю его в контроллер. Подумал нужно обрабатывать ошибки, если с базой косяк будет и возник вопрос что вернуть в случае провала.

[Дмитрий Ковальский]

Сергей Коновалов: А вот это уже надо было писать в вопросе. На мой взгляд в интерфейс надо бросить некое исключение "DataSourceException". Чтобы пользователю показать что что-то пошло не так, а не "Ничего не найдено".
Совет №1 - передайте в метод хоть какие-то параметры фильтрации. Фильтровать на сервере приложения - плохо. Таскать данные, которые вам не нужны - плохо. Вам точно нужен ВЕСЬ(!!!) список адресов? Как правило даже если это так - выводить весь вам не надо, а нужно пагинировать штук по 20.

[Сергей Коновалов]

Адресов всего может быть порядка 20-30, так что показать в данном случае лучше все, но спасибо, учту для других сущностей

[Сергей Коновалов]

Кстати насчет goto? можете показать пример как его тут использовать? и если я не ошибаюсь, его вроде не рекомендуется использовать?

[Дмитрий Ковальский]

Сергей Коновалов: Я к тому что использовать NULL, в паре с ловлей NullReferenceException - чистое применение goto. Код начнет прыгать туда сюда и отладка станет праздником жизни. Лучше отловить конкретные исключения - ошибки чтения данных с выводом конкретной инфы -что-то не так с базой или еще чем-то. NULL для этих целей в принципе можно использовать, но на мой взгляд лучше все-же четко отделять логику работы и возникшие ошибки.Проверка на NULL в данном случае будет проверкой на то что была ошибка и к бизнес-логике никакого отношения не имеет.

[abs0lut]

Сергей Коновалов: Это был сраказм

[Валерий Абакумов]

Дмитрий Ковальский - не согласен с Вами в том, что всегда лучше возвращать пустой список. Очень часто бизнес-логика бывает такой сложной, что избежать null-объектов почти невозможно. Добавьте сюда еще и ограниченные сроки реализации задач. Разработчику нужно постоянно следить за тем, чтобы не было null-объектов, а по мне - это сущий геморрой. Из личного опыта - гораздо проще обрабатывать null-ситуации (здесь я имею в виду проверку на null, а не обработку NullReferenceException), чем пытаться уйти от null. Все равно никогда от null-ситуаций Вы не уйдете. И проще проверить на null. Тем более, что в C# 6.0 появилась такая замечательная конструкция, как ?.

[Дмитрий Ковальский]

Валерий Абакумов: Спасибо за ваш комментарий, в общем и целом вы правы. Однако есть тонкости. 1) бесспорно null-ситуации возможны. В конце концов поля базы данных допускают Null-значения, и, более того, в C#/.NET есть такая вещь как интерфейс INullable и Nullable-типы, однако это не повод самому себе дополнительно придумывать приключения. 2) и САМОЕ ВАЖНОЕ - автор вопроса подразумевает не возможность вернуть NULL при пустой выдаче, а именно ошибочное поведение. Я убежден что надо строго разделять пустую выдачу и ошибку чтения.Исключительные ситуации в асп, к слову, можно и нужно логировать. Задушенный на раннем этапе Exception с возвратом NULL приведет к тому что в логике контроллеров,страниц мом представлений появится код типа

if(list !=null{working...}) else{наверно что-то пошло не так,что? а хрен знает. Может прав нету, может база не читается, может словарь прокис}

[Валерий Абакумов]

Дмитрий Ковальский ну да, мой комментарий больше ушел в сторону общего использования null. А что касается данного конкретного вопроса, то я считаю, что в данном методе GetAdresses() при ловле исключения в блоке catch нельзя возвращать ни null, ни Enumerable.Empty. Только throw (вместе с логированием).

Answer 2

[Vitaliy Orlov]

Конечно пустую коллекцию.. у тебя даже об этом сигнатура говорит. В случае с Null ты не сможешь передать результат дальше (в функцию принимающую List<Adresses>) без дополнительных проверок. А вообще, очень многое зависит от требования и контекста, в последнем случае, например, можно кидать соответствующий exception (говорящий о пустом списке), который дальше уже будет обрабатываться. Но, мне кажется логичней вернуть пустую коллекцию заданного типа.

Answer 3

[Антон Папин]

Лучше вернуть OperationResult в обоих случаях, например такой:

public class OperationResult<T>
{
     T Result { get; set; }
     string Message { get; set; }
     bool IsSuccessful { get; set; } // можно заменить на enum с бОльшим количеством вариантов
}

Answer 4

[Валерий Абакумов]

Добрый день!

Правильным будет следующий вариант:

public IEnumerable<Adresses> GetAdresses()
        {
            try
            {
                 return dbContext.Adresses.ToList();
            }
            catch (Exception ex)
            {
                // Логируем ошибку - что-то вроде:
                //_loggerService.Error(ex);
                throw;
            }
        }

А снаружи, например, у Вас будет метод, который при ловле исключения будет выдавать что-то осознанное пользователю:

public ActionResult Addresses()
        {
            try
            {
                 var addressList = _dataService.GetAdresses();
                 return View("_Addresses", addressList);
            }
            catch (Exception ex)
            {
                TempData["Error"] = "Не удалось загрузить список адресов. Попробуйте позже."
                 return View("_Addresses");
            }
        }

Во View я бы тоже советовал всегда делать проверку на null, потому что по независящим от Вас причинам Model спокойно может быть null. И лучше на клиенте такие ситуации обрабатывать, чтобы у Ваших пользователей не было ступора и они не перестали пользоваться Вашим веб-приложением.

<div>
@if(Model != null && Model.Any())
{
   <div>
       @foreach(var address in Model)
       {
           @*Здесь вывод Ваших адресов*@
       }
   </div>
}
else
{
    <div>
        @TempData["Error"]
    </div>
}
</div>