Рассудите спор

[varan]

Я специально не скажу своё мнение на этот счет пока что

Вот такой кейс. В классе 18 раз повторяется строка, примерно такая.

if ($this->verbose)
$this->log($this->verboseStream, (переменные, текстовые сообщения и т.д.));

в других 18 местах тоже самое, только переменные в логи идут другие.

вопрос:
1) надо ли это упрощать (подозрение на дублирование кода) и если да, то как?
2) или не надо, и так вполне нормально

 

[fixxxer]

PSR-3, $this->logger->info (или $this->logger->debug, смотря что в данном контексте значит verbose).

 

[флоппик]

$this->log($verbose_level, $stream, $message)...

 

[whirlwind]

Правило трех ударов забыли. Третье повторение повод для рефакторинга. $this->logger->info сильно привязывает код к реализации. Судя по тому, что их 18, определенно это нужно завернуть, как указал флоппик. Только еще стрим выпилить из прототипа, он же повторяется.

 

[fixxxer]

whirlwind, на PSR-3 то уж можно и завязаться

 

[AmdY]

varan, а почему проверку if ($this->verbose) в саму функцию log

 

[whirlwind]

whirlwind, на PSR-3 то уж можно и завязаться

А оно не противоречит. Внутренние шоткаты нужно делать всегда, если есть дублирование. Эти сокращения делают код более понятным и устойчивым к изменениям в принципе. Единственная причина этого не делать это оверхед на вызовы. Но это вопрос об экономии на спичках.

 

[Breeze]

$this->log($verbose_level, $stream, $message)...

в лучших традициях php прототип надо вот такой

$this->log($message[, $verbose_level[, $stream]])

 

[Вурдалак]

Правило трех ударов забыли. Третье повторение повод для рефакторинга. $this->logger->info сильно привязывает код к реализации. Судя по тому, что их 18, определенно это нужно завернуть, как указал флоппик. Только еще стрим выпилить из прототипа, он же повторяется.

Судя по тому, что их 18, то это скорее повод для декомпозиции класса. А $this->log() потребует писать это как

$this->log(LogLevel::INFO, ...)

ну да, шибко удобнее.

 

[fixxxer]

А оно не противоречит. Внутренние шоткаты нужно делать всегда, если есть дублирование. Эти сокращения делают код более понятным и устойчивым к изменениям в принципе. Единственная причина этого не делать это оверхед на вызовы. Но это вопрос об экономии на спичках.

Тогда уж $this->logVerbose($message), а то шило на мыло.

 

[varan]

Собственно, все уже высказались походу.
Моё мнение было такое: что это во-первых god object, который надо разбивать на части, а во-вторых надо добавить метод $this->verboseLog, чтобы улучшить читабельность.

 

[whirlwind]

Вурдалак, fixxxer, Это все частности. Как хотите сокращайте, но здесь через шоткат нужно. Это вообще везде такое правило насчет трех ударов. Например с переменными то же самое. Если локально третий раз какая нибудь хрень типа $this->some->any вылазит, это повод сократить через дополнительную локальную переменную. Профит стартует с третьего повторения. Элементарная арифметика.

Когда то о чем я говорю будет плохо? Тогда, когда log реализуется внутри этого класса. Это локально увеличивает сложность настолько, что задолбаешься тестировать все варианты. По этому реализация по PSR-3 - через декомпозицию это единственно правильно. Инжектим логгер и мокаем либо стаббим. Но с точки зрения внутреннего дизайна класса это должен быть приватный метод-шоткат делегирующий логгеру.

 

[varan]

ну то есть в итоге, отдельный метод + отдельный логгер?
Есть принципиальные возражения у кого-нибудь?

 

[Вурдалак]

Я не вижу никакого смысла в отдельном методе: если я вызываю $this->logger->debug() 3 раза, то насрать; если 15 раз, то нужна декомпозиция класса, а не отдельный метод. Например, декоратор, который будет заниматься только логгированием. Тут нужно смотреть конкретный случай. А то тут уже личные предпочтения уже преподносятся как правило.

 

[AmdY]

Ай, я не понимаю почему у автора возник спор, ясен пень, повторяющийся иф это плохо, потому хорошая практика - собирать статистику о копипасте в CI. А как улучшить, вам должно быть виднее изнутри, а то Вурдалак, с whirlwind опять устроят холивар о тёплом и мягком.

 

[varan]

> Ай, я не понимаю почему у автора возник спор, ясен пень, повторяющийся иф это плохо
я тоже считаю что это ясен пень. Но вот мой коллега утверждает, что тупо каждый повторяющийся иф пихать в метод или куда-то еще. Потому что разницы нет, где будет слово verbose: в ифе или в названии метода.

 

[AmdY]

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

 

[MiksIr]

> Ай, я не понимаю почему у автора возник спор, ясен пень, повторяющийся иф это плохо
я тоже считаю что это ясен пень. Но вот мой коллега утверждает, что тупо каждый повторяющийся иф пихать в метод или куда-то еще. Потому что разницы нет, где будет слово verbose: в ифе или в названии метода.

Ладно бы if повторяющийся, но вам еще флаг verbose таскать придется по классам всем. Тогда как в случае метода он будет в одном месте.

 

[whirlwind]

Я вообще не понимаю, что за проблема с этим verbose. Ведь черным по белому написано, что это внутрь того метода идет ибо каждый раз иф. Зачем об этом маячить в сигнатуре? Умолчать и забыть! Вот по аналогии с другими ЯП типа java с каким нибудь slf4j, наверняка это предварительная оптимизация типа if debug enabled then cast everything to the string and concatenate the ducking strings. Типа заранее озаботились такой серьезной проблемой. Да забейте вы на эту конкатенацию внутри логгера или что там еще мешает вам просто молча завернуть все это дерьмо в один метод.

 

[WMix]

а логгер не хочет принять параметр verbose один раз, через тот же DI?