Это нормальный код?

[Exploits]

Ребят, делаю магазин. Купил модуль. Не буду говорить какой, но мне его надо переделать.

Так вот полез в контроллер и там так оформлен код:

if со скобками else - без, ну ногу можно поломать там..

Ну вот еще

теги в контроллере, ну зачем?

в общем то код читабельный но глаза как-то режет все это.

 

Кто, что скажет, это нормально так, или я придираюсь к оформлению кода?

[Eldaeron]

echo 'Число ';
if ($number < 0) {
	echo 'отрицательное';
} else echo 'положительное';

 

[Eldaeron]

Жесть форум лагает. Да нормальный код, каждый привык как привык писать. Ему удобно так)

[chukcha]

Ну, за такое можно конечно только пальчиком погрозить

 

echo 'Число ';
if ($number < 0) {
	echo 'отрицательное';
} else echo 'положительное';

 

version_compare

уже отменили?

 

[Eldaeron]

21 минуту назад, chukcha сказал:

Ну, за такое можно конечно только пальчиком погрозить

 

echo 'Число ';
if ($number < 0) {
	echo 'отрицательное';
} else echo 'положительное';

 

version_compare

уже отменили?

 

Каждый по своему делает. Видел даже строку с версией в массив перегоняли и сравнивают [0] и [1] елементы массива, и ничего работает)

 

Код ревью нужно проводить? Ну незнаю, я например видел как делают функции и в каждой с них:

 такой кусок кода:

if (isset($this->request->get['filter_category'])) {
			$url .=  '&filter_category=' . $this->request->get['filter_category'];
		}

Вместо того этот кусок кода можно 1 раз только поставить для всех функций, и уменшить свой код на 30-60%

[Gorman]

Не psr-2, конечно, но более менее читаемо Мешанина из отсутствия и присутствия фигурных скобок в условии, тернарных и обычных вариантов if-а в двух строчках - ниче хорошего с точки зрения красоты точно, но иногда это не самое главное. Привести код к красоте проще, чем его написать+запустить

[ArtemPitov]

1 час назад, Exploits сказал:

Кто, что скажет, это нормально так, или я придираюсь к оформлению кода?

 

Ну это больше философский вопрос 

// Если кода не много, вполне можно 
if ($a > 0) $b = true; 
else $b = false;

// Но лучше вот так 
$b = ($a > 0) ? true : false;

// Но вот такой вариант для чтения удобнее 
if ($a > 0) { 
	$b = true;
} else {
	$b = false;
}

// Но если у нас только булевое значение, можно вообще вот так 
$b = $a > 0;

 

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

[chukcha]

2 минуты назад, Eldaeron сказал:

я например видел как делают функции и в каждой с них:

 такой кусок кода:

Это к Даниелю..

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

[Exploits]

Нет, ну я знаю что можно писать по разному и ничего в этом нет плохого. Но я к тому что зачем переплетать один стиль с другим? Где-то есть, где-то нет.

Код работает и это самое главное но где эстетика?) Opencart сам же в таком стиле

if($a){
  $b = $a;
}else{
  $b = $c;
}

а не так:

if($a){
  $b = $a;
}else
  $b = $c;

мне кажется второй вариант как-то похабно выглядит, вроде как на скорую руку накидано.

 

Понятно что 

$a = $b?$b:$c;

это удобно и читаемо, можно смешивать. Но когда то есть скобки то нет, как-то не то, мне так кажется.

 

А по поводу тегов в контроллере, по-моему это не очень красиво, можно же разделять

 

Пост создал услышать ваши мнения, вот это классно написано:

// Если кода не много, вполне можно 
if ($a > 0) $b = true; 
else $b = false;

// Но лучше вот так 
$b = ($a > 0) ? true : false;

// Но вот такой вариант для чтения удобнее 
if ($a > 0) { 
	$b = true;
} else {
	$b = false;
}

// Но если у нас только булевое значение, можно вообще вот так 
$b = $a > 0;

 

[Gorman]

Не так давно пришел в Опенкарт, раньше работал по большей части с фреймворками, где контроллеры максимально тонкие. Увидел контроллер OC с выгрузкой каждой переменной из language файла и отравкой в $data массив, сразу захотелось добавить две строчки кода для быстрой автоматической загрузки lang во view.

 

Зашел в языковой класс (system/library/language.php) и смутился - вот прямо перед глазами два метода: all() и merge(), чтобы взять все из твоего lang файла и отправить во вью.

Подгрузка всей языковой даты выглядела бы так:

$data = $this->language->all();
$this->language->merge($data);

При этом, над ними висит комментарий: 

// Please dont use the below function i'm thinking getting rid of it.

То есть деприкатный метод. Это история для версии 2.3. 

Зашел в 3.0.2, merge() теперь нет (но не очень-то и нужен, повторял нативную php функцию), all() до сих пор висит, но уже без строчки "не пользоваться", и в некоторых контроллерах из коробки ее уже пользуют.

 

И вот думаешь - зачем столько копипаста в каждом контроллере, когда есть замечательный метод автоимпорта текстов в view? Никаким DRY-ем и не пахнет 

Так что опенкарт сам подталкивает временами к какой-то бездумной не структурированной работе, и приходится этому сопротивляться)
 

Edited February 17, 2018 by Gorman

[Otvet]

2 часа назад, Exploits сказал:

теги в контроллере, ну зачем?

 

чтоб не пихать через окмод килобайты текста в шаблон 

какая разница как сделано в своем отдельном контроллере если это не создает тормозов/конфликтов/угроз

[Exploits]

10 минут назад, Otvet сказал:

 

чтоб не пихать через окмод килобайты текста в шаблон 

какая разница как сделано в своем отдельном контроллере если это не создает тормозов/конфликтов/угроз

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

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

[chukcha]

2 часа назад, Gorman сказал:

Зашел в 3.0.2,

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

По большому счету, даже all не нужен
$data = $this->load->language

в случае первого использования

$data = array_merge($data, $this->load->language

2 часа назад, Exploits сказал:

А по поводу тегов в контроллере,

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

 

В pagination теги не смущают?

 

Даже в языковых переменных теги не смущают?

[Exploits]

1 час назад, chukcha сказал:

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

По большому счету, даже all не нужен
$data = $this->load->language

в случае первого использования

$data = array_merge($data, $this->load->language

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

 

В pagination теги не смущают?

 

Даже в языковых переменных теги не смущают?

Ну языки то само собой, например strong span и т.д. это нормально т.к. по сути то текст.

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

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

Но в целом, услышав мнение сообщества понял что это нормально, хотя как по мне то лучше придерживаться стандартов. Работает конечно и так и так.

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

[chukcha]

1 час назад, Exploits сказал:

В пагинации же нет своего шаблона,

Как это?

		$output = '<ul class="pagination">';

		if ($page > 1) {
			$output .= '<li><a href="' . str_replace(array('&amp;page={page}', '?page={page}', '&page={page}'), '', $this->url) . '">' . $this->text_first . '</a></li>';

правильно было бы отдать массив ссылок, а не отрендеренный список, а внешний видом управлять в теме
например $this->load->view('common/pagination', $pagination)

[Exploits]

3 минуты назад, chukcha сказал:

Как это?

		$output = '<ul class="pagination">';

		if ($page > 1) {
			$output .= '<li><a href="' . str_replace(array('&amp;page={page}', '?page={page}', '&page={page}'), '', $this->url) . '">' . $this->text_first . '</a></li>';

правильно было бы отдать массив ссылок, а не отрендеренный список, а внешний видом управлять в теме
например $this->load->view('common/pagination', $pagination)

Это да, я к тому что нет своего tpl

Вот так

$this->load->view('common/pagination', $pagination)

было бы в стиле Opencart. Почему так не сделали непонятно.