Стоит ли переписывать полностью метод в данной ситуации?

Question

[driverx18]

Есть метод, например: getUsers. Он юзается в 10-20 местах в проекте, и мне вдруг понадобилось получать юзеров отсортированных не по айдишнику (как сейчас), а по колонке order. В голове есть два варианта:
1) Сделать у существующего метода getUsers необязательный параметр (withSorting = false), и в тех самых 10-20 местах оно будет работать как сейчас, потому что там по дефолту проставляется фолс, а в нужном нам месте поставить true. Но мне говорили, что если добавляем всякие такие флаги то это служит нарушением SRP.
2) Создать метод getUsersSorted, например. Но как по мне, это тупо копирование метода getUsers с добавлением ордер бай.

Хотел бы спросить у знающих, как и почему лучше сделать?

Comments

[Антон]

Я бы более, добавил параметр params, который делал бы гибче весь запрос за счет добавления его в выборку WHERE и все, что далее

[dmitriy]

Антон, тем самым вы завяжите ваше приложение к полям бд, что создаст неудобства в будущем

[Толстый Лорри]

необязательный параметр (withSorting = false)

Из названия не ясно, по какому именно признаку будет сортировка.

если добавляем всякие такие флаги то это служит нарушением SRP

В данном случае метод getUsers как и раньше будет возвращать пользователей, поэтому SRP это не нарушит.

[devalone]

Какой-то странный подход. В php нет ORM? В go-pg например запросы выглядят так https://github.com/go-pg/pg/wiki/Writing-Queries очень удобно, просто на месте пишешь, что хочешь получить и не городишь тыщи методов.

[batyrmastyr]

devalone, сотни их, бывают и получше вашего.
Но если навешиваешь несколько одинаковых условий в куче мест, то логично вынести это в какую-нибудь функцию.

Answer 1

[vista1x]

Сделайте общий метод getUsersQuery(), который просто возвращает пользователей без какой либо сортировки. Свой метод getUsers() измените так, что бы он использовал первый метод. Плюс, добавьте метод getUsersSorted(), где будете возвращать данные, отсортированные в нужном виде. Не зная структуры проекта сложно написать какие то примеры кода, но я бы сделал примерно так:

function getUsersQuery() {
    // ...
}
function getUsers() {
    return getUsersQuery()->orderBy('id');
}
function getUsersSorted() {
    return getUsersQuery()->orderBy('name');
}

Answer 2

[ThunderCat]

Вариантов 3:
1) Добавить параметр: плюсы - просто, можно добавить гибкий параметр, например $sortField. минусы - в следующий раз понадобится менять еще и направление сортировки - прийдется снова костылить.
2) Отдельный метод - уже лучше, но все равно рано или поздно функционал надо будет менять и с параметрами что -то прийдется делать.
2.1) Передавать в качестве аргумента некий массив/объект настроек, плюсы - гибко, минусы - каждый раз надо учитывать/знать структуру объекта настроек, что не очень удобно.
3) Использовать внутренние паблик свойства объекта для настройки работы логики объекта, плюсы - не надо менять инерфейс вызова, достаточно сделать

$users->sortBy = 'order'; $users->sortDir = 'desc'; $users->getUsers();

. Минусы - сходу не вижу, у кого есть идеи - отпишитесь в комментах.

Comments

[dmitriy]

сортировка по нескольким полям?

[Lander]

dmitriy,

$users->sort = [
    'order' => 'desc',
    'name' => 'asc'
];

[Sergei Iamskoi]

dmitriy, вместо строки передавать массив: ['id' => 'desc', 'name' => 'asc'];

[dmitriy]

Sergey Yamskoy если сортировка нужна по join таблице ?

[Sergei Iamskoi]

dmitriy, а если начинаются "а если" - использовать orm.

[vism]

Sergey Yamskoy, предлагаю сразу сделать метод, в который надо массивом передавать список таблиц, сортировки, поиски и все все все.
1 метод на все приложение, зачем мелочиться и пердевать всего лишь ['id' => 'desc', 'name' => 'asc'];

[vism]

Кстати что и требовалось доказать, залайкали очень странный ответ, который еще автоматов и решением будет.
Если уж городить небоскребы - то лучше как dmitriy написал. Это едиственно верный сложный вариант

[Sergei Iamskoi]

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

[vism]

Sergey Yamskoy, А ну да. Класс в котором будут все все все поля и надо его заполнять..

[ThunderCat]

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

[vism]

ThunderCat, Ну да, а в вашем подходе залюбитесь разбираться, какого фига с этом сервисе какие-то сортировки. Или какого хрена надо постоянно пересоздавать сервис или объект с иным назвачением, чтоб не тянут хистори предыдущих настроек.

Если в мэйлере увидели что-то, это не значит для оно для слоя бизнес логики сработает.

[ThunderCat]

vism, я не рекомендовал это как сильвербуллет, но для коллекций например(а судя по контексту человеку нужно получить коллекцию) это вполне верный подход. Что делать - передавать объект дататрансфера или работать с внутренними свойствами - это уже дело вкуса, особых выгод не вижу.

[vism]

ThunderCat, Стоит ли переписывать полностью метод в данной ситуации?
Вот все описал человек.
Дело не вкуса, а будущей поддержки кода. Вариант делать внутренними свойствами - это пуля стреляющая в свою ногу.

Answer 3

[vism]

Вобщем вы за ответом пришли туда, где 95% евда мидл-джуны.

Варианты ранее предложеные либо сильно меняют структуру, либо переусложняют.

Просто создаете второй метод и общее тело (формирование запроса) выносите в третий приватный/протектид метод. (вот vista1x не поленился и даже код накидал примерный)
В созданном методе заодно оставляете аргумент для сортировки.
Нужен будет еще - создаете третий.

Понимаете, что таких появится еще много - тогда уже меняете структуру, рефакторите.

Comments

[dmitriy]

а можно сразу сделать правильно

[vism]

dmitriy, можно, но не нужно. Переусложнее это не есть качество.
Код должен быть как можно более простым.
Это архитекутара логики долна быть заложена сразу, а код должен иметь гибкость на несколько шагов вперед, а не на 100.
А то так hello word можно пару месяцев кодить, заклыдваю "правильную" архитектуру кода

[dmitriy]

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

[vism]

dmitriy, Я про array не писал) Я за то, чтоб заказчик не оплачивал допустим 10 часов разработки, когда можно сделать за 1 час и потом это место скорее всего никогда не изменится)

[dmitriy]

Я про array не писал) Я за то, чтоб заказчик не оплачивал допустим 10 часов разработки, когда можно сделать за 1 час и потом это место скорее всего никогда не изменится)

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

Answer 4

[dmitriy]

public function getUsers(?UserCriteria $criteria){
//logic create query
if($criteria && $criteria->getSortedByOrder()){
//logic sorted by order, $criteria->getSortedByOrder() return null|"DESC"||"ASC"
}
else{
//default sorted
}
}

Comments

[vism]

Ну уж если вы за "сразу нормально делать", то дефолтная сортировка тоже должна быть в criteria.

Сказать почему там или сами догадаетесь?

[dmitriy]

скажите

[vism]

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

Если критерия решает задачу по сортировке, то она и должна её решать. А если критерия не передана, то брать её по умолчанию (getDefaultCriteria())

[dmitriy]

мне более нравится подход когда Criteria выступает в роли DTO и лишь служит для передачи данных, а наполнением данных занимается фабрика, и пусть будет она DefaultCriteriaFactory которая будет наполнять сортировкой по умолчанию или UserCriteriaFactory, AdminCriteriaFactory , и тд

[NaumovInnokentiy253]

dmitriy, Извините что не по теме, но что значит знак вопроса в

public function getUsers(?UserCriteria $criteria){}

[dmitriy]

null или UserCriteria

[vism]

dmitriy, Да, абсолютно согласен и я про это)

Боюсь только автор вопроса такое реализовать не сможет верно (иначе бы не было бы вопроса :) )
Ну пусть хоть заинтересуется и отложится у него как правильно. Может книги почитает по паттернам кода.

Answer 5

[John_Nash]

Однозначно 1й вариант, т.к.
изменения исходного кода минимальны,
новое условие легко убирается/модифицируется

Answer 6

[Александр Лыкасов]

driverx18 В вашем случае я бы последовал варианту №3 в ответе ThunderCat

Answer 7

[Иван]

https://en.wikipedia.org/wiki/Method_chaining вам в помощь :)
Например в Eloquent это могло бы выглядеть так (запрос с active=true так для примера):
User::where('active', true)->orderBy('name', 'ASC')
Т.е. вы по необходимости выстраиваете цепочки методов.

Answer 8

[morricone85]

Вообще, для того, что бы многократно использовать подобные методы, как getUsers, нужно передавать такие аргументы как:
- поле сортировки;
- направление сортировки;
- к-во выводимых элементов;
- c какого элемента начинать.

Это - необходимый минимум. Поэтому, самый практичный способ решения - передача массива настроек, как и указывает ThunderCat в своем комментарии.

То есть, как это может выглядеть:

function getUsers($arr = null) {
    if (!empty($arr['order'])) {
        $order = $arr['order'];
    }  else {
        $order = 'primary column';
   }
...
}

А, при вызове метода, например, так:

getUsers('order' => 'date_created', 'limit' => 50);

Comments

[dmitriy]

данный метод очень плохо поддерживать, маштабируемости функционала на нуле, поясню, когда в
getUsers($arr = null)
может прилететь все что угодно, например $arr['Order'] или $arr['ORDER']? вам нужно городить валидаторы ваших конфигов,
а также вы завязываете работу приложения на поля базы, в дальнейшем манипуляции с полями бд приводят к полному рефакторингу, кода который использует выборку,
опять же ваш IDE не будет подсказывать весь набор полей котрый программист должен запихнуть в массив, вы каждый раз будете ходить в метод и капать что и как правильно назвать

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

->getUsers(
UserCriteria::create()->setLimit(50)->setOrderByCreate("ASC")
);

вы скажете зачем усложнять, обясню: это окупится потом, когда вы будете писать функционал проверки,корректности ваших массивов, а также их докумтацию, чтобы я не смог сделать
->getUsers(['Order' => 'date_created'], 'limit' => 50);
->getUsers(['Order' => 'dateCreated'], 'limit' => 50);
->getUsers(['Order' => 'date_created'], 'limit' => "2,50");