С#. Код ревью метода

22 февраля 2022, 04:30

Метод:

private string GetValuesFromJObjectsArray<T>(IEnumerable<T> objects)
{
    var type = typeof(T);
    var list = new string[] { };
    if (type == typeof(JProperty))
    {
        list = (objects as IEnumerable<JProperty>).Select(f => f.Name).ToArray();
    }
    else if (type == typeof(JToken))
    {
        list = (objects as IEnumerable<JToken>).Select(f => (f.Parent as JProperty).Name).ToArray();
    }
    return $"Correct values: { String.Join(", ", list) }. [ Total elements: {objects.Count()}]";
}

Метод работает корректно - он принимает коллекцию JProperty или JToken. Далее вытягивается Name элементов и преобразуется полученный масив в строку. К сожалению у этих двух объектов нету общих предков, поэтому мне пришлось напистаь else if.

Если бы вы проводили код ревью, на что Вы бы обратили внимание / что Вам не понравилось здесь?

Спрашиваю в целях самообразования

P.S. Я надеюсь я правильный тег прицепил ("инспекция кода")

Спасибо

Answer 1

Если метод применим всего для двух типов - довольно странным решением кажется создание именно Generic метода, я бы все таки создал два отдельных метода.

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

Также objects.Count() здесь происходит лишний проход по набору. Если будут методы обрабатывающие только наборы допустимых типов - можно будет взять значение свойства Length у полученного массива, т.к. его длина будет равна количеству элементов в исходном наборе.

Answer 2

У вас идет обработка только 2 типов JToken и JProperty как код должен себя вести, если попадется какой-то другой тип? Возможно вы подразумеваете, что других типов туда не попадет, но сейчас это не более, чем подозрение. Хорошим тоном будет бросить исключение, или какой-нибудь лог. Так как ваш код все-таки имеет конкретное возвращаемое значение, то лучше все-таки исключение.
Сейчас ваш код плохо расширяем относительно новых типов. Добавление еще одного типа потребует собой добавление отдельного if/else if. Кроме того, с каждым новым типом количество осуществляемых проверок будет увеличиваться. Как потенциальное решение, можно сделать Dictionary <type, Func<string>>, который будет заполняться (инициализироваться) при создании класса, а сам метод GetValuesFromJObjectsArray будет осуществлять лишь поиск по ключу в словаре. В такой реализации, для каждого нового типа нужно будет лишь добавить значение в словарь, при этом сама реализация метода останется без изменений.
Можно поработать над неймингом: что означает заветное "J" в названии классов и самого метода?