Рефакторинг - нужен совет

[wildsam]

Есть вот такой небольшой симпатичный класс:

class Processor
{
  // Класс прогружает с сайта элементы и их дочек.

	const FIRST_LEVEL_URL = 'http://site.com/1.php';
	const SECOND_LEVEL_URL = 'http://site.com/2.php?id=';

	protected $_downloader;
	protected $_storage;
	protected $_firstLevelRepsonceParser;
	protected $_secondLevelRepsonceParser;
	
	// IDownloader реализует единственный метод get($url), IStorage - add($id, $name, $parentId), IResponceParser - parse()
	public function __construct (IDownloader $downloader, IStorage $storage, IResponceParser $firstLevelResponceParser, IResponceParser $secondLevelReponceParser)
	{
		$this -> _downloader = $downloader;
		$this -> _storage = $storage;
		$this -> _firstLevelResponceParser = $firstLevelResponceParser;
		$this -> _secondLevelResponceParser = $secondLevelResponceParser;
	}

	public function go()
	{
		$firstLevelResponce = $this -> _downloader -> get (self::FIRST_LEVEL_URL);                          // закачали весь файл
		$firstLevelItems = $this -> _firstLevelResponceParser -> parse($firstLevelResponce);                // получили array целевых данных 
		foreach ($firstLevelItems as $firstLevelItem) {                                                     // для каждого элемента...
			$this -> _storage -> add ($firstLevelItem.id, $firstLevelItem.Name, 0)                            // добавили в хранилище (id, name, parentId)
  
	  		$secondLevelResponce = $this -> _downloader -> get (self::SECOND_LEVEL_URL.$firstLevelItem.id);   // закачали весь файл с дочерними элементами
		  	$secondLevelItems = $this -> _secondLevelResponceParser -> parse($secondLevelResponce);           // получили array целевых данных
		  	foreach ($secondLevelItems as $secondLevelItem) {                                                 // для каждого элемента...  
				$this -> _storage -> add ($secondLevelItem.id, $secondLevelItem.Name, $firstLevelItem.id)       // добавили в хранилище (id, name, parentId)
	  		}
		}
	}

}

Одно плохо: инъекция практических однотипных конкретных реализаций одного интерфейса IResponceParser $firstLevelResponceParser, IResponceParser $secondLevelReponceParser в конструкторе.

Вопрос: как избавиться? Возможно с радикальной сменой реализации.

Спасибо!

 

[флоппик]

public function __construct (IDependencyContainer $idc)
{
    $this->_dc = $idc;
}

 

[wildsam]

Я не хочу использовать Dependency Container или Service Locator. Больше работы при создании моков при тестировании, неявный контракт и куча всего другого. Плюс, в этом конкретном случае, в контейнер, грубо говоря, засовывается все, что не получилось грамотно разрешить в архитектуре. Ничего хорошего в этом нет.

Вопрос открыт.

 

[Dovg]

флоппик
хайрез есть?

 

[Ragazzo]

флоппик
на этом вся symfony построена и кстати это вроде fuck yeah

 

[флоппик]

флоппик
хайрез есть?

неа, это первое, что в гугле нашлось по «Close enough»

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

 

[Ragazzo]

флоппик
я говорил про то что это картинка не "close enought", а изначально была "fuck yeah" вроде хотя потом стало непонятно и смешались люди, кони

 

[hell0w0rd]

Я не хочу использовать Dependency Container или Service Locator. Больше работы при создании моков при тестировании, неявный контракт и куча всего другого. Плюс, в этом конкретном случае, в контейнер, грубо говоря, засовывается все, что не получилось грамотно разрешить в архитектуре. Ничего хорошего в этом нет.

Вопрос открыт.

А вы di по назначению используйте, а не передавайте в класс. И все будет прекрасно мокаться
А вообще скажите, нафига вам конструктор?
Также я не понимаю о каком тестировании идет речь, если у вас url захардкоден, а вложенность строится с помощью переменных.
я бы убрал константы и переменные вложенности, и передавал бы массив...
И вообще, если класс - processor, зачем в него передавать объекты, реализующие методы процессора? Они еще где-то используются?
Если сменится логика - вы будете менять тело класса, а этого не должно происходить. То есть если добавится третий уровень вложенности - вы просто должны изменить аргументы, а не класс

 

[wildsam]

А вы di по назначению используйте, а не передавайте в класс. И все будет прекрасно мокаться
А вообще скажите, нафига вам конструктор?
Также я не понимаю о каком тестировании идет речь, если у вас url захардкоден, а вложенность строится с помощью переменных.
я бы убрал константы и переменные вложенности, и передавал бы массив...
И вообще, если класс - processor, зачем в него передавать объекты, реализующие методы процессора? Они еще где-то используются?
Если сменится логика - вы будете менять тело класса, а этого не должно происходить. То есть если добавится третий уровень вложенности - вы просто должны изменить аргументы, а не класс

Собственно, этот класс предназначен для работы с одним-единственным сайтом и жестко завязан на его структуру. Универсальность не нужна.

Конструктор - на случай будущего рефакторинга основного метода с extract method (а он обязательно будет, т.к. явно слишком громоздкий).

Про тестирование - меня на данном этапе интересуют изолированные TDD-юниттесты. Мокаются именно внешние ресурсы.

Логика не будет менятся, приложение одноразовое. Но мне хотелось бы на будущее разобраться с тем, как победить вот такую конкретную не особо красивую инъекцию конкретных классов, имеющих общего предка.


Можно поподробнее про "А вы di по назначению используйте, а не передавайте в класс. И все будет прекрасно мокаться". На мой взгляд, у меня самая что ни есть обычная DI.

Спасибо за интерес к вопросу!

 

[hell0w0rd]

wildsam с моей, воспаленной, точки зрения - класс должен быть как можно менее привязан к подобным вещам. Пусть он работает с ответом представленным в определенной форме, к примеру.
TDD и юнит-тесты - вещи взаимосвязанные, но в одну сторону. TDD - включает юнит-тесты, а вот юнит тесты могут быть сами по себе, как в вашем случае. TDD - сначала тесты, потом код.
Смысл DI в том, что зависимости не обращаются к контейнеру на прямую и когда вам посоветовали передать DIC в класс - это было неправильно с точки зрения паттерна DI

 

[Koc]

флоппик
на этом вся symfony построена и кстати это вроде fuck yeah

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

 

[AmdY]

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

по очкереди. нужно избавиться от комментариев.

$firstLevelResponce = $this -> _downloader -> get (self::FIRST_LEVEL_URL);      
// меняем на
$firstLevelResponce = $this->getFirstLevel();

хотя, зачем эта временная переменная, которая используется лишь для следующего конвеера.

// выносим в отдельный метод getFirstLevelItem
$firstLevelResponce = $this -> _downloader -> get (self::FIRST_LEVEL_URL);                          // закачали весь файл
return $this -> _firstLevelResponceParser -> parse($firstLevelResponce);   
// заменяем на 
$firstLevelItems = $this->getFirstLevelItem();

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

В реальности нужно разделить получение данных и их сохранение. обычно требуется разносить долгий download и быстрые parse-save по разным очередям.

 

[keltanas]

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

Немного оффтопа