[ Поиск ] - [ Пользователи ] - [ Календарь ]
Полная Версия: Код-ревью SetCMS
Страницы: 1, 2, 3
twin
Обещал пару моментов показать, которые вызывают вопросы. Выполняю. :)

Самое начало, index.php
<?php

define('INC', 1);

require_once 'vendor/autoload.php';
error_reporting(E_ALL);

session_start();

$app = new \SetCMS\App(__DIR__);

$request = \Zend\Diactoros\ServerRequestFactory::fromGlobals();

$response = $app->run($request, new \Zend\Diactoros\Response());

foreach ($response->getHeaders() as $name => $values) {
foreach ($values as $value) {
header(sprintf('%s: %s', $name, $value), false);
}
}


echo $response->getBody();


1. Ты же вроде был противником постоянно стартанутой сессии. Странно видеть в твоем коде session_start(); Особенно когда есть заявка на PSR-7

2. По PSR-7. Не, я конечно понимаю, что не в твоих правилах заботиться о ресурсах. Хотя это вроде и опенсорсная CMS. Но раз уж взялся внедрять zend-diactoros, то почему бы не сделать как положено, через потоки, тем более, что там для этого все есть? Почему там банальное echo? Я уже не спрашиваю о заголовках, хотя не нашел нигде в коде приведение их к RFC.

Ну вот, пару моментов обозначил. :)

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
Цитата (twin @ 13.08.2017 - 19:44)
1. Ты же вроде был противником постоянно стартанутой сессии. Странно видеть в твоем коде session_start(); Особенно когда есть заявка на PSR-7


Это осталось от рефакторинга, раньше вообще было вот так https://bitbucket.org/cheevauva/setcms/src/...le-view-default. Позже выделю в объект и буду стартовать сессию только по обращению.

Цитата (twin @ 13.08.2017 - 19:44)
2. По PSR-7. Не, я конечно понимаю, что не в твоих правилах заботиться о ресурсах. Хотя это вроде и опенсорсная CMS. Но раз уж взялся внедрять zend-diactoros, то почему бы не сделать как положено, через потоки, тем более, что там для этого все есть? Почему там банальное echo? Я уже не спрашиваю о заголовках, хотя не нашел нигде в коде приведение их к RFC.

это типа
file_put_contents('php://output', $string);

если, да, то не вижу большой разницы, все равно то и то делает одно и тоже в данном случае.

Цитата (twin @ 13.08.2017 - 19:44)
Ну вот, пару моментов обозначил. :)

чет как-то не густо, где обещанная лапша?

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Цитата (chee @ 13.08.2017 - 18:40)
чет как-то не густо, где обещанная лапша?
Я боюсь тратить время напрасно, обычно все заканчивается тем, что я для тебя не авторитет, и к моему мнению прислушиваться нет смысла. Во всяком случе в плане архитектуры. Я посмотрю код дальше, но на уровне алгоритмов. Архитектуру трогать не буду.
Цитата (chee @ 13.08.2017 - 18:40)
если, да, то не вижу большой разницы, все равно то и то делает одно и тоже в данном случае.
Нет, не это. Ты просто не до конца разобрался в PSR-7. Одно из его преимуществ - экономия памяти на буфер. Вот здесь посмотри, должно понятно быть.

Про заголовки здесь.

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
Цитата (twin @ 13.08.2017 - 22:56)
Нет, не это. Ты просто не до конца разобрался в PSR-7. Одно из его преимуществ - экономия памяти на буфер. Вот здесь посмотри, должно понятно быть.

Про заголовки здесь.

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

Цитата (twin @ 13.08.2017 - 22:56)
Я посмотрю код дальше, но на уровне алгоритмов.

Основная логика пока что тут
https://bitbucket.org/cheevauva/setcms/src/...le-view-default

https://bitbucket.org/cheevauva/setcms/src/...ator/?at=setcms

https://bitbucket.org/cheevauva/setcms/src/...le-view-default

https://bitbucket.org/cheevauva/setcms/src/...le-view-default

https://bitbucket.org/cheevauva/setcms/src/...le-view-default

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Цитата (chee @ 13.08.2017 - 19:09)
Основная логика пока что тут
Хорошо, но я по порядку, мне так легче с концепцией будет разобраться.
Вот тут я не совсем понял. Ты перезаписываешь старые файлы где то, или влет исполняешь? Или это вообще временный класс?


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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
Цитата (twin @ 13.08.2017 - 23:15)
Вот тут я не совсем понял. Ты перезаписываешь старые файлы где то, или влет исполняешь? Или это вообще временный класс?


старые файлы исполняются без перезаписи, раньше был eval, ебрал потом что оказался бесполезным. Это не временный класс, он отвечает за исполнение старого кода.

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Понятно. Завтра продолжу. Одна ремарочка только. Чтобы почище было, я бы так сделал:
    public function includeFile($_filename)
{
setcms($this->app);

$level = error_reporting(E_ALL & ~E_NOTICE);

...........


error_reporting($level);
}
И просто совет еще. Я бы этот класс назвал наоборот: AppLegacy. Они тогда рядом в папке будут. Слету вникать удобнее. :)

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
twin, два дельных совета, даже сейчас сделаю коммит.

https://bitbucket.org/cheevauva/setcms/comm...ac8b6ad6cd2fccb

Вот кстати еще плюс ООП, переименовывать методы и классы в IDE очень приятно, IDE сама найдет места использования и переименует.

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Чёт я в смятении. Вот этот метод ввел меня в ступрор. Ты говоришь, что класс для работы со старым кодом. Логично предположить, что это новый код. Но... Так ли это? Стоит его разбирать?

Цитата (chee @ 13.08.2017 - 19:44)
Вот кстати еще плюс ООП, переименовывать методы и классы в IDE очень приятно, IDE сама найдет места использования и переименует.
Ты удивишься, но ООП тут не. при. чем. smile.gif

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
twin
Объясни пожалуйста (это не критика, я реально хочу понять вашу логику), для чего классу App контракт?

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
twin
Винда хоть и масдай, но все же еще используется. smile.gif Вот тут чище было бы DIRECTORY_SEPARATOR.

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
Цитата (twin @ 14.08.2017 - 05:07)
для чего классу App контракт?

Потому что он инжектируется в другие объекты и может быть в будущем подменены наследуемым от себя классом.
Цитата (twin @ 14.08.2017 - 05:34)
Вот тут чище было бы DIRECTORY_SEPARATOR.

Если работает, зачем мне париться? При этом всём я на винде сейчас разрабатываю и это работает. Винда воспринимает и так \ и так /.

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Цитата (chee @ 14.08.2017 - 03:01)
Потому что он инжектируется в другие объекты и может быть в будущем подменены наследуемым от себя классом.
Понятно. YAGNI нарушаем-с smile.gif
Цитата (chee @ 14.08.2017 - 03:01)
Если работает, зачем мне париться? При этом всём я на винде сейчас разрабатываю и это работает. Винда воспринимает и так \ и так /.
Ну вообще я так понял, что у проекта не исключено светлое будущее... Все же чище было бы с константой. Как говорил мой дед - нужно всё делать хорошо. Плохо, оно само получится.
И на треий вопрос ты не ответил. Код метода AppLegacy::loadContext() - legacy, или стоит разобрать?

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
chee
Цитата (twin @ 14.08.2017 - 09:13)
Все же чище было бы с константой.

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

Цитата (twin @ 14.08.2017 - 09:13)
Код метода AppLegacy::loadContext()

Это код из старой версии (выдел из старого index.php https://bitbucket.org/cheevauva/setcms/src/...e-view-default). Код в будущем я планирую переписать, но сейчас у меня к нему позиция - работает худо бедно и ладно.

Моя основная задача на данный момент, сделать полную эмуляцию старого кода. Когда я завершу её, то начнется рефакторить код, который написал для эмуляции. В том числе и loadContext описанный выше.

_____________
Люди, имеющие низкий уровень квалификации, делают ошибочные выводы, принимают неудачные решения и при этом неспособны осознавать свои ошибки в силу низкого уровня своей квалификации
twin
Цитата (chee @ 14.08.2017 - 05:34)
Код в будущем я планирую переписать, но сейчас у меня к нему позиция - работает худо бедно и ладно.
Ясно. Не буду трогать.
Цитата (chee @ 14.08.2017 - 05:34)
или у него есть изъяны, которые приведут к багам, тогда я конечно же поправлю.
Странная у тебя логика. Контракт ты на будущее пишешь, а тут не видишь ничего дальше этого метода. Но у тебя же есть и второй, как я вижу - публичный. getPath() который. А значит им можно воспользоваться для получения пути. И это логично. Но вот если потом вдруг понадобится выделить часть этого пути, начнется геморрой. И вот это вовсе не нарушение YAGNI, как с контрактом. Это дальновидность и чистый код. smile.gif Но дело конечно хозяйское.

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

Нужно уважать мнение оппонета. Ведь заблуждаться - его святое право.

Настаивал, настаиваю и буду настаивать на своем. На кедровых орешках.

user posted image
Быстрый ответ:

 Графические смайлики |  Показывать подпись
Здесь расположена полная версия этой страницы.
Invision Power Board © 2001-2024 Invision Power Services, Inc.