В контроллере более 1000 строк кода. Это правильно?

Question

[atis //]

Контроллер содержит бизнес логику. Проверку введенных данных и т.д.
Так же содержит более 35 экшенов.
Правильно ли это? Может разбить контроллер на несколько частей?

P.S. контроллер от админки!

Answer 1

[Котик Антон]

Размер файла ещё не говорит о том, что в нём что-то не правильно.

Контроллер содержит бизнес логику. Проверку введенных данных и т.д.

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

Может разбить контроллер на несколько частей?

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

Answer 2

[Дмитрий Ковальский]

Ну вообще многовато, но бывает. Что значит "содержит бизнес логику"? Общается с базой данных запросами? тогда это большой грех. Надо выносить в отдельные классы и компоненты. Опять же сама цифра 35 мало что значит. Возможно ряд экшенов надо выносить в отдельные контроллеры просто по смыслу,а не от переизбытка методов в одном.

Comments

[atis //]

Бизнес логику, ну проверку введенных данных.

[Дмитрий Ковальский]

atis //: Проверка данных != бизнес-логика. В идеале ваш action должен быть похож на что-то вроде

if(params.isValid())
{
modelObject.someMethod(params);
}
return something;

А бизнес-логика уже внутри класса Модели, ну и дальше в глубь в зависимости от архитектуры приложения. На выходе получите 35 action и сотня-полторы строк кода.

[atis //]

О. спасибо чувак.

Answer 3

[Сергей Зеленский]

ну 35 action это нормально что поделать, а вот бизнес-логику надо держать в модели а на счет проверки данных лучше вообще ее делать отдельным модулем

Answer 4

[Евгений Елчев]

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

Answer 5

[atis //]

Экшены в контроллере.
Это самые маленькие!

/**
	 * Добавляем производителя
	 * 
	 * @param string|null $result
	 */
	public function addProducer($result = null)
	{
		// проверка на авторизацию
		$this->logged();

		if (isset($_POST['submit'])) {

			$Producer = new Producer(new DataBase());
			$res = $Producer->addProducer(1, $_POST['producer']);
			$res = ($res === true) ? 'success' : 'error';

			header('Location: ' . _HTTP_SERVER . 'admin/add-producer/' . $res);
			exit;
		}

		$data = [];
		$data['result'] = $result;

		$this->renderAdminPage('add_producer', $data);
	}

	// ------- AJAX -------

	/**
	 * Удаление производителя
	 * @param int|null $producerId
	 */
	public function delProducer($producerId = null)
	{
		// проверка на авторизацию
		$this->logged();

		if (isset($_POST['producerId'])) {
			
			$Producer = new Producer(new DataBase());

			echo json_encode(['result' => $Producer->delProducer($_POST['producerId'])]);
		}
	}

Comments

[Евгений Елчев]

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

[atis //]

Все подряд: добавление товаров, производителей, цен.
Кароче 1 контроллер на всю админку.

[heartdevil]

А у вас в экшенах параметры не передаются от post/get? Надо обязательно из POST читать? Еще странно, что у вас в каждом экшене $this->logged(); проверка идет. Может это можно было один раз на входе класса проверить? Это так мысли в слух)

[atis //]

неаааааа.(
фигово спроектировали. на yii писать не дали(

[atis //]

Точнее GET передаются как параметры, POST - вытягивай сам