Я тут недавно поревьюил по сторонней просьбе некоторый код на C#,что было тестовым заданием в некоторую контору. Автор программит профессионально более 10 лет.
Так вот, я молчу про то, что в бизнес логике вызываются напрямую всякие методы UI отрисовки вида увеличить прогресс бар. Тут ладно, придираюсь. Нет тестов вообще — ладно, тоже нормально (хотя почему то у меня даже на тестовое задание всегда требовали тесты). Все отправлено в виде одного коммита в GIT, и не видно истории как все делалось — допустим тоже так и надо. Ну и всякие GOD methods.
Но мне вот интересно, это я такой непродвинутый и это только меня напрягает?
Короче, важно быстродействие, кое что выполняется в цикле. По смыслу — выполнение функции, один вход, один выход. Однако автор создает экземпляр определенного класса через new без параметров и далее выполняет функцию calculate, тоже без параметров. После чего созданный объект забывается и в цикле создается новый.
Я был только сторонний наблюдатель и на это посмотрел после того, как это было отправлено, просто как сторонний консультант по просьбе автора. Непосредственно тех, кто оценивал тестовое задание — все удовлетворило. Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
Здравствуйте, elmal, Вы писали:
E>Я был только сторонний наблюдатель и на это посмотрел после того, как это было отправлено, просто как сторонний консультант по просьбе автора. Непосредственно тех, кто оценивал тестовое задание — все удовлетворило. Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
Отдельный класс подразумевает разделение области ответственности, что зачастую ожидается и верно.
Возможно, его следовало сделать статиком, если никакого состояния не предполагалось.
Здравствуйте, elmal, Вы писали:
E>Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
это же не рокет саенс (а бизнес логика привариваемая к UI), там тысячу (условно) таких объектов класса в цикле создай — никто и не заметит разницы, а оптимизации ради оптимизаций — это уже маразм ..
хотя (но мы код конечно не видели) вероятно кодерок рукожоп который (если мы правильно тебя поняли) не умеет в статические классы и методы (не знаю как там с ними в C#), но если брать и напр., "другие ньюансы того же C/C++" — в них тоже не все шарят, или не до конца (взять тот же Boost засранный шаблонами (в исходном коде), в котором чёрт ногу сломит — а то и даже сами разработчики, потому в семье не без урода в каждой версии — не без ошибок)
но, и в целом умельцев красиво (и грамотно) писать код — "единицы" (условно) .. остальные — отъявленные рукожопы ..
Здравствуйте, elmal, Вы писали:
E>Я тут недавно поревьюил по сторонней просьбе некоторый код на C#,что было тестовым заданием в некоторую контору. Автор программит профессионально более 10 лет.
E>Так вот, я молчу про то, что в бизнес логике вызываются напрямую всякие методы UI отрисовки вида увеличить прогресс бар. Тут ладно, придираюсь. Нет тестов вообще — ладно, тоже нормально (хотя почему то у меня даже на тестовое задание всегда требовали тесты). Все отправлено в виде одного коммита в GIT, и не видно истории как все делалось — допустим тоже так и надо. Ну и всякие GOD methods.
E>Но мне вот интересно, это я такой непродвинутый и это только меня напрягает?
E>Короче, важно быстродействие, кое что выполняется в цикле. По смыслу — выполнение функции, один вход, один выход. Однако автор создает экземпляр определенного класса через new без параметров и далее выполняет функцию calculate, тоже без параметров. После чего созданный объект забывается и в цикле создается новый.
E>Я был только сторонний наблюдатель и на это посмотрел после того, как это было отправлено, просто как сторонний консультант по просьбе автора. Непосредственно тех, кто оценивал тестовое задание — все удовлетворило. Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
придираетесь))
жестянщик не обязан уметь строить ракеты как ракетчик, хотя оба работают с материалами.
скорость проф роста также зависит от способностей, возможностей, приоритетов.
Здравствуйте, elmal, Вы писали:
E>Я был только сторонний наблюдатель и на это посмотрел после того, как это было отправлено, просто как сторонний консультант по просьбе автора. Непосредственно тех, кто оценивал тестовое задание — все удовлетворило. Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
Идеологически — создание нового экземпляра класса верно и оправданно. Если мне нужна новая табуретка, то надо сделать новую табуретку, а не брать табуретку, которую я сделал год назад и пытаться ее переделать под требования к новой табуретке.
Практически — ужасающе, конечно. Тем более, если я правильно понял, там класс совсем несложный, и static экземпляр решил бы все проблемы. Либо создать до цикла.
До поры до времени никто в цикле и не стал бы делать, разве что совсем уж начинающие. Памяти было мало , удаление ручное, быстродействие маленькое.
Ну а сейчас всем объяснили, что память не ресурс, что GC все сделает, что предварительная оптимизация зло и т.д. В общем, не парьтесь, делайте так, а если выяснится потом, что это и есть bottleneck, то тогда и отрефакторите.
Увы, это стало почти что нормой и объяснить , что это плохо, очень трудно.
Здравствуйте, elmal, Вы писали:
E>Я тут недавно поревьюил по сторонней просьбе некоторый код на C#,что было тестовым заданием в некоторую контору. Автор программит профессионально более 10 лет.
А на чём программит последние 3-5?
E>Но мне вот интересно, это я такой непродвинутый и это только меня напрягает?
Кроме гита, всё правильно говоришь. Народ совсем разучился или не умел писать на ОО языках в ОО парадигдме. Нарушение SRP, GOD классы, неумение сделать объектную декомпозицию хоть как-то. Просто берут и фигачат код. Такое ощущение, что никто не читал книг по ООП вообще, ни книг по архитектуре и не применяли это на практике.
E>Короче, важно быстродействие, кое что выполняется в цикле. По смыслу — выполнение функции, один вход, один выход. Однако автор создает экземпляр определенного класса через new без параметров и далее выполняет функцию calculate, тоже без параметров. После чего созданный объект забывается и в цикле создается новый.
Это коммент как минимум в код ревью средней сиверити, т.е. new в цикле даже на C# это не особо хорошо.
E>Я был только сторонний наблюдатель и на это посмотрел после того, как это было отправлено, просто как сторонний консультант по просьбе автора. Непосредственно тех, кто оценивал тестовое задание — все удовлетворило. Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
А разве static методов C# нет? Взяли его на работу в итоге?
Здравствуйте, elmal, Вы писали:
E>Короче, важно быстродействие, кое что выполняется в цикле. По смыслу — выполнение функции, один вход, один выход. Однако автор создает экземпляр определенного класса через new без параметров и далее выполняет функцию calculate, тоже без параметров. После чего созданный объект забывается и в цикле создается новый.
Такой стиль пропагандирует Мартин в своём Чистом Коде. Если у нас есть функция с n локальными переменными и 100500 строчек кода, то можно из неё сделать отдельный класс, локальные переменные сделать полями, а код декомпозировать по методам.
В языках, позволяющих определять функции внутри функций, этот подход устарел.
Здравствуйте, scf, Вы писали:
scf>Такой стиль пропагандирует Мартин в своём Чистом Коде. Если у нас есть функция с n локальными переменными и 100500 строчек кода, то можно из неё сделать отдельный класс, локальные переменные сделать полями, а код декомпозировать по методам. scf>В языках, позволяющих определять функции внутри функций, этот подход устарел.
Там нет n локальных переменных. Там ровно один параметр на входе и один на выходе, после чего никакое внутреннее состояние объекта не интересно. По существу там чистая функция, и что то я сомневаюсь что Мартин бы такое посоветовал. static метод определенного класса полностью бы решил все проблемы (пусть даже и с созданием экземпляра внутри, ибо там Thread Safe требуется).
Здравствуйте, Pavel Dvorkin, Вы писали:
PD>Увы, это стало почти что нормой и объяснить , что это плохо, очень трудно.
Тут проблема то ИМХО не в быстродействии, а в раздувании количества строчек и кода на ровном месте.
То есть вместо одной сторочки:
var result = Calculator.Сalculate(data)
или даже
var result = Сalculate(data)
, на ровном месте появляется две:
var calculator = new Calculator();
var result = calculator.calculate(data);
Здравствуйте, elmal, Вы писали:
E>Здравствуйте, Pavel Dvorkin, Вы писали:
PD>>Увы, это стало почти что нормой и объяснить , что это плохо, очень трудно. E>Тут проблема то ИМХО не в быстродействии, а в раздувании количества строчек и кода на ровном месте.
E>То есть вместо одной сторочки:
E>
E>var result = Calculator.Сalculate(data)
E>
E> или даже E>
E>var result = Сalculate(data)
E>
E>, на ровном месте появляется две:
E>
E>var calculator = new Calculator();
E>var result = calculator.calculate(data);
E>
E>И ведь не лень ...
ну будут 2 строчки не в одном месте, а в разных, что изменится. реальная проблема здесь только в лишнем выделении памяти в цикле.
Здравствуйте, elmal, Вы писали:
E>Там нет n локальных переменных. Там ровно один параметр на входе и один на выходе, после чего никакое внутреннее состояние объекта не интересно. По существу там чистая функция, и что то я сомневаюсь что Мартин бы такое посоветовал. static метод определенного класса полностью бы решил все проблемы (пусть даже и с созданием экземпляра внутри, ибо там Thread Safe требуется).
Тогда похоже на типичные симптомы синдрома Александреску.
Здравствуйте, Pavel Dvorkin, Вы писали:
PD>Идеологически — создание нового экземпляра класса верно и оправданно. Если мне нужна новая табуретка, то надо сделать новую табуретку, а не брать табуретку, которую я сделал год назад и пытаться ее переделать под требования к новой табуретке.
Однако как создавать? Если брать табуретку, то идеологически наверно у нас есть фабрика табуреток, которой мы скармливаем необходимые материалы и говорим сделай табуретку. И фабрика табуреток у нас одна и нам не нужно для каждой табуретки сначала стрить фабрику. Здесь же самое близкое — это мы создаем объект табуретка и у этого объекта дергаем метод создайся. А на деле там еще веселее. Нужно, условно говоря, посчитать значение какой формулы. На входе выражение, на выходе число. Логически с точки зрения использования одна функция, причем чистая функция. Но практически создают новый экземпляр класса интерпретатора выражений с одним методом "вычислить". Хорошо хоть не додумались этому интерпретатору выражений сначала посылать сообщение "получи входные данные", потом сообщение "посчитай", а потом посылать сообщение отдай "результат". А то ведь и до такого можно было додуматься в теории.
Я просто несколько удивился, что можно проработать в индустрии более 10 лет, на разных работах, на буржуйских заказчиков, и при этом за подобное не получать по рукам и не делать по умолчанию наиболее удобно к использованию.
Здравствуйте, elmal, Вы писали:
E>Отсюда вопрос, я что — действительно слишком придираюсь, или по крайней мере в C# является бест практикой на каждый чих создавать класс с одним публичным методом и чтоб вызывающий код для простейшего случая не вызывал одну функцию, а вынужден сначала создавать объект оператором new, а затем вызывать один единственный метод чтоб что то посчитать?
Норм. Разработчик реализовал алгоритм и инкапсулировал его в класс — нормальная практика.
Это особенно удобно, если логика сложная (разбивается на несколько функций) и/или логика требует некоторой параметризации (передается в конструктор).
Минусы статиков по сравнению с обычным классом:
— если логика сложная — то все вспомогательные функции тоже должны быть статик
— если появятся параметры класса, то придется все статики переделать в нормальные методы (а параметры обязательно появятся)
— код прибит к классу — никакого тестирования, никакого IoC
Обычный (не статик) класс легко превратить в статик — просто создав единственный экземпляр класса (ага, синглетон), и обращаться к нему.
Это тоже не очень хороший вариант, но он имеет меньше перечисленных выше проблем.
P.S. Ну и в целом, класс с одним публичным методом — нормальная практика при использовании IoC и DI
ДОБАВЛЕНО:
Понятно, что для "простейшего случая" это все может быть оверхедом — ведь все зависит от ситуации, от требований. Если случай был уж совсем простой, и можно было ограничиться одной простой чистой функцией, то предлагаю считать, что разработчик просто предомонстрировал в тестовом задании умение работать с таким паттерном (инкапсуляция алгоритма в класс). Тоже имхо не плохой вариант.
Здравствуйте, Буравчик, Вы писали:
Б>Минусы статиков по сравнению с обычным классом: Б>- если логика сложная — то все вспомогательные функции тоже должны быть статик
Никаких проблем, это вообще не минус.
Б>- если появятся параметры класса, то придется все статики переделать в нормальные методы (а параметры обязательно появятся)
С чего им появляться? Это класс — функция. В теории там если что и появится, то конфиг. Конфиг можно передать вторым аргументом при выполнении функции или параметрами по умолчанию.
Б>- код прибит к классу — никакого тестирования, никакого IoC
Тестирование элементарное — это чистая функция, один вход, один выход. Никаких сайдэффектов вообще, тестирование идеальное и образцовое, даже сторонних зависимостей и ввода вывода нет.
Б>P.S. Ну и в целом, класс с одним публичным методом — нормальная практика при использовании IoC и DI
Да, нормальная. Но IoС и DI не использовались.
ЮЗдравствуйте, elmal, Вы писали:
E>Однако как создавать? Если брать табуретку, то идеологически наверно у нас есть фабрика табуреток, которой мы скармливаем необходимые материалы и говорим сделай табуретку. И фабрика табуреток у нас одна и нам не нужно для каждой табуретки сначала стрить фабрику.
Ну этот вопрос спорный. Как городской житель я, конечно, закажу табуретку фабрике. Но сельский житель вполне возможно сделает ее сам. В прошлом году жена пристала — сделай табуретку. Сделал ей, квадратную. Так она в каком-то журнале увидела круглую табуретку и теперь требует такую. Квадратная ей больше не нужна. Пытаюсь сделать из квадратной круглую — что-то плохо получается .А фабрики в деревне нет.
>Здесь же самое близкое — это мы создаем объект табуретка и у этого объекта дергаем метод создайся. А на деле там еще веселее. Нужно, условно говоря, посчитать значение какой формулы. На входе выражение, на выходе число. Логически с точки зрения использования одна функция, причем чистая функция.
Верно, чистая функция. Статический метод класса, если о Java речь идет
>Но практически создают новый экземпляр класса интерпретатора выражений с одним методом "вычислить". Хорошо хоть не додумались этому интерпретатору выражений сначала посылать сообщение "получи входные данные", потом сообщение "посчитай", а потом посылать сообщение отдай "результат". А то ведь и до такого можно было додуматься в теории.
Ну вообще-то небольшой нюанс тут есть. На static можно нехило проколоться в многопоточном приложении. Добавишь статическое поле — и привет.
E>Я просто несколько удивился, что можно проработать в индустрии более 10 лет, на разных работах, на буржуйских заказчиков, и при этом за подобное не получать по рукам и не делать по умолчанию наиболее удобно к использованию.
Все зависит от того, чем именно он занимался.
Где-то такое приведет к приличной просадке скорости или проблемам с памятью. А где-то ничего заметного не будет. Например, в GUI — пока приложение вызовет ОС для отрисовки, а ОС перебросит все это видеодрайверу, а тот будет думать, как это лучше отрисовать, потому что у него таких приложений много, а потом начнет шрифты рендерить.. На этом фоне лишний new — что слону дробина.
Здравствуйте, Pavel Dvorkin, Вы писали:
PD>Ну этот вопрос спорный. Как городской житель я, конечно, закажу табуретку фабрике. Но сельский житель вполне возможно сделает ее сам. В прошлом году жена пристала — сделай табуретку. Сделал ей, квадратную. Так она в каком-то журнале увидела круглую табуретку и теперь требует такую. Квадратная ей больше не нужна. Пытаюсь сделать из квадратной круглую — что-то плохо получается .А фабрики в деревне нет.
Под фабрикой я подразумеваю не паттерн фабрика, а тот, кто создает определенные экземпляры. Табуретка не будет создавать сама себя, табуретку создает что то (станок), или кто то (конкретный человек или даже коллектив). Соответствующие методы создания будут уже относиться к конкретным другим классам, причем зачастую через интерфейс CanCreateStool с одним методом. При этом сам класс табуретка имплементить такое не может ибо это будет концептуальная ошибка. Это если действительно заморачиваться с истинным ООП, лично я крайне ленив и предпочитаю писать минимум кода и все эти фабрики я пишу только при крайней необходимости, когда от их наличия кода оказывается меньше, а не как блин некоторые, пихают эти фабрики везде чтоб показать как они круто ООП знают и понимают, и раздувают код на ровном месте в разы.
PD>Ну вообще-то небольшой нюанс тут есть. На static можно нехило проколоться в многопоточном приложении. Добавишь статическое поле — и привет.
Можно, но нечего использовать по существу глобальные переменные без крайней необходимости. static предполагает что никаких статических полей использоваться не будет, только константы.
PD>Где-то такое приведет к приличной просадке скорости или проблемам с памятью. А где-то ничего заметного не будет. Например, в GUI — пока приложение вызовет ОС для отрисовки, а ОС перебросит все это видеодрайверу, а тот будет думать, как это лучше отрисовать, потому что у него таких приложений много, а потом начнет шрифты рендерить.. На этом фоне лишний new — что слону дробина.
Я могу ошибаться, то именно относительно производительности сам копилятор и оптимизатор в теории может этот new и не делать. А даже если и сделает, то это будет первое поколение объектов накладные расходы на сборку мусора и создание довольно ничтожны. А вот то, что вызывающий код будет вынужден создавать объект и затем у него вызывать метод — это уже более неприятно, ибо это заставляет пользователя писать больше кода на ровном месте.
Ну у меня такое обычно получается в процессе рефакторинга, т.е.
static int Calculate(int x);
сначала вырастает в:
static int Calculate(int x, int y, int z, double a, out string c);
потом разбивается на:
static int Calculate(int x, int y, int z, double a, out string c)
{
int tmp1 = CalculateSomething(x, y, z);
int tmp2 = CalculateSomethingElse(tmp1, z, a);
c = CalculateSomeAuxiliaryOutput(tmp1, tmp2);
return tmp2;
}
и потом, чтобы не засорять область видимости временными CalculateSomething(), и чтобы не передавать вручную по 10 параметров, упрощается в:
class Calculator
{
readonly int X, Y, Z, A;
public string C{get; private set;}
public Calculator(int x, int y, int z, double a) {...}
private int CalculateSomething();
public int Calculate()
{
int tmp1 = CalculateSomething();
int tmp2 = CalculateSomethingElse(tmp1);
CalculateSomeAuxiliaryOutput(tmp1, tmp2);
return tmp2;
}
}
Если Calculate() хоть сколько-либо нетривиально, лишнее выделение памяти здесь погоды не сделает.
Здравствуйте, scf, Вы писали:
scf>Такой стиль пропагандирует Мартин в своём Чистом Коде. Если у нас есть функция с n локальными переменными и 100500 строчек кода, то можно из неё сделать отдельный класс, локальные переменные сделать полями, а код декомпозировать по методам. scf>В языках, позволяющих определять функции внутри функций, этот подход устарел.
Победа ООП над силами разума.
Здравствуйте, Quebecois, Вы писали:
Q>Если Calculate() хоть сколько-либо нетривиально, лишнее выделение памяти здесь погоды не сделает.
out string выглядит как кандидат на логирование или на что еще, с выводом промежуточных результатов куда либо. Лично я для этого использую подход шины сообщений. В случае, если я живу без DI IOC фреймворков, у меня есть глобальный доступный отовсюду объект eventBus, с двумя методами — publish и subscribe (ладно, трeмя — unsubscribe может еще потребоваться). Соответственно там, где нужно уведомить внешний код, я в расчетной части буду кидать соответствующие события. Ну а если я уже живу с фреймворками, то функционал шины сообщений там как правило уже встроен, ну и у меня уже никаких статиков — все классы конструируются и инжектятся зависимости уже фреймворком, соответственно у меня вообще никаких new в коде практически нет.
Собственно с точки зрения внешнего кода подход с классом все равно чуть более громоздкий — один хрен нужно передавать параметры, теперь м их передаем в конструкторе и можем еще дополнительно просетать всякие второстепенные параметры отдельно. Преимуществ подобного подхода особо не вижу. А если мы останавливаемся на статике и не хотим в промежуточные функции передавать до черта аргументов — мы внутри статического метода конструируем объект (точнее даже структуру зачастую, ибо методы не нужны) контекста выполнения и уже контекст выполнения передаем на подрасчеты. Но при этом вся сложность и все проблемы самого расчета клиента вызывающего кода не касаются никак, для вызывающего кода это чистая функция без сайдэффектов.
Есть конечно еще один подход — параметры передавать через билдеры, возвращая ссылку на сам билдер чтобы можно было в цепочку инициализации выстраивать. Но такое ИМХО нужно использовать далеко не по умолчанию и 100 раз подумать, нужно таким заморачиваться или нет смысл усложнять клиентский код, лично я такое считаю оправданным только в какой тяжелой библиотеке на этапе инициализации один раз во всем приложении.
(а бизнес логика привариваемая к UI), там тысячу (условно) таких объектов класса в цикле создай — никто и не заметит разницы, а оптимизации ради оптимизаций — это уже маразм .. 
остальные — отъявленные рукожопы .. 
.А фабрики в деревне нет.
