Jump to content
Search In
  • More options...
Find results that contain...
Find results in...
  • Sign Up

Recommended Posts

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

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

kV22DpCjTeGgXmGbcjuhfQ.png

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

Ну вот еще

8RW0IbE3TKCCIABEYqCGng.png

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

qvJVoZEgRgi8Hmr7nLUM-A.png

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

 

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

Link to post
Share on other sites
echo 'Число ';
if ($number < 0) {
	echo 'отрицательное';
} else echo 'положительное';

 

Link to post
Share on other sites

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

Link to post
Share on other sites

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

 

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

 

version_compare

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

 

Link to post
Share on other sites
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%

Link to post
Share on other sites

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

Link to post
Share on other sites
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;

 

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

  • +1 2
Link to post
Share on other sites
2 минуты назад, Eldaeron сказал:

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

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

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

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

Link to post
Share on other sites

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

Код работает и это самое главное но где эстетика?) 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;

 

Link to post
Share on other sites

Не так давно пришел в Опенкарт, раньше работал по большей части с фреймворками, где контроллеры максимально тонкие. Увидел контроллер 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 by Gorman
Link to post
Share on other sites
2 часа назад, Exploits сказал:

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

 

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

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

Link to post
Share on other sites
10 минут назад, Otvet сказал:

 

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

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

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

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

Link to post
Share on other sites
2 часа назад, Gorman сказал:

Зашел в 3.0.2,

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

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

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

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

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

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

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

 

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

 

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

Link to post
Share on other sites
1 час назад, chukcha сказал:

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

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

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

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

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

 

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

 

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

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

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

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

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

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

Link to post
Share on other sites
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)

Link to post
Share on other sites
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. Почему так не сделали непонятно.

Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
  • Recently Browsing   0 members

    No registered users viewing this page.

×
×
  • Create New...

Important Information

On our site, cookies are used and personal data is processed to improve the user interface. To find out what and what personal data we are processing, please go to the link. If you click "I agree," it means that you understand and accept all the conditions specified in this Privacy Notice.