Комментарии 31
НЛО прилетело и опубликовало эту надпись здесь
Ресайз самый обычный, лучше бы swtich'om сделали. вместо^
-1
Извините, что с php не знаком вообще, но, надеюсь, взгляд стороннего будет полезен.
Строки
«if ($highest == 0) {
$this->e->add(6, 'highest:'. $highest. 'px');
return false;
}»
смущают. Зачем нужна склейка с заведомо известным $highest == 0?
Так же хотел узнать — php, кажется, поддерживает обработку исключений, но вы ее почему-то не используете.
Кроме того, стоит использовать enum или их аналоги вместо хардкодных строк-вариантов (например, в $proportion)
Строки
«if ($highest == 0) {
$this->e->add(6, 'highest:'. $highest. 'px');
return false;
}»
смущают. Зачем нужна склейка с заведомо известным $highest == 0?
Так же хотел узнать — php, кажется, поддерживает обработку исключений, но вы ее почему-то не используете.
Кроме того, стоит использовать enum или их аналоги вместо хардкодных строк-вариантов (например, в $proportion)
-1
Прочитав статью habrahabr.ru/blogs/development/51518/ про исключения, я для себя сделал вывод, что в избытке использовать исключения для вывода сообщений пользователю / записью в базу -> плохо.
Можно поподробнее про enum?
Можно поподробнее про enum?
0
Постараюсь. Не совсем уверен, уместно ли это в комментах — по вашему желанию можем переместиться в личную переписку.
Кратко, enum — это перечисление заведомо известных констант, строковых, числовых или объектных. Как-то так:
public static class Proprtions {
public const string AUTO = «auto»;
public const string BY_HEIGHT = «height»;
public const string BY_WIDTH = «width»;
}
После чего в коде не нужно писать строки, а можно явно указывать, что вы имели в виду: например, Proportions.AUTO. Помимо избавления от синтаксических ошибок, это позволяет более логично писать код — например, ничто не мешает объявить еще одну константу DEFAULT и сделать ее равной AUTO. Код будет содержать больше логики, и меньше деталей реализации.
Про exceptions — советую почитать не только дискуссии на Хабре, но вам виднее.
Класс events вы не очень удачно назвали — стоит переименовать его в EventLog, например. Кроме того, так и не понял, зачем вам singleton.
Кратко, enum — это перечисление заведомо известных констант, строковых, числовых или объектных. Как-то так:
public static class Proprtions {
public const string AUTO = «auto»;
public const string BY_HEIGHT = «height»;
public const string BY_WIDTH = «width»;
}
После чего в коде не нужно писать строки, а можно явно указывать, что вы имели в виду: например, Proportions.AUTO. Помимо избавления от синтаксических ошибок, это позволяет более логично писать код — например, ничто не мешает объявить еще одну константу DEFAULT и сделать ее равной AUTO. Код будет содержать больше логики, и меньше деталей реализации.
Про exceptions — советую почитать не только дискуссии на Хабре, но вам виднее.
Класс events вы не очень удачно назвали — стоит переименовать его в EventLog, например. Кроме того, так и не понял, зачем вам singleton.
+1
Есть альтернатива enum'у в ряде случаев. Использовать методы несущие в своих названиях значения констант. Для логов, например: void saveSuccessMsg(string $msg), таким образом наружу торчат понятные имена простых функций, а все тонкости флажков и состояний — спрятаны в классе.
Синглтон тут в тему, ибо не нужны другие объекты на основе этого класса. В принципе сообщение в лог должно быть stateless, то есть — не иметь свойств необходимых для хранения. (Если, например, не считается количество сообщений за одно обращение к коду). То есть можно написать просто класс с набором статических функций
Синглтон тут в тему, ибо не нужны другие объекты на основе этого класса. В принципе сообщение в лог должно быть stateless, то есть — не иметь свойств необходимых для хранения. (Если, например, не считается количество сообщений за одно обращение к коду). То есть можно написать просто класс с набором статических функций
0
Простите, второго класса не видел, но зачем целый класс, когда есть GD и один, фактически, метод для ресайза?
В эвентах вы смешали логику и представление. Кодирование конкретного вида (HTML-тегов) внутри класса это очень и очень плохо. Что бы клиенту вашего класса поменять внешний вид, ему прийдется лезть внутрь и менять. К тому же вы выводите данные непосредственно (echo), что явно непозволительно в большинстве случаев.
Так же общая нечитабельность кода, например, return-ы в разных местах функции, при сопровождении много времени теряется только на восстановление логики работы функции.
О фигурной скобке на той же линии говорить не буду, но просто сами посмотрите на свой код, например функции get(), в пастебине и попробуйте посмотреть в случае:
if ()
{
}
что было бы намного читабельнее.
Опять же прямой SQL-код внутри класса, что недопустимо, у клиента может быть другая база, другие таблицы (хотя бы названия даже, у многих с префиксами). То есть, ваше решение абсолютно не конфигурабельно, а следовательно не юзабельно.
И несколько других замечаний.
В целом, могу предположить, что вы недавно сели за пхп, и за ООП в частности. Слишком это в глаза бросается. Здесь много работы по рефакторингу и оптимизации, а кроме того, главный вопрос: а есть ли уже готовые решения? Попробуйте задать его себе, посмотреть, как у людей сделано.
И напоследок, зачем к имени класса добавлять {}? И так ясно, что это класс.
В эвентах вы смешали логику и представление. Кодирование конкретного вида (HTML-тегов) внутри класса это очень и очень плохо. Что бы клиенту вашего класса поменять внешний вид, ему прийдется лезть внутрь и менять. К тому же вы выводите данные непосредственно (echo), что явно непозволительно в большинстве случаев.
Так же общая нечитабельность кода, например, return-ы в разных местах функции, при сопровождении много времени теряется только на восстановление логики работы функции.
О фигурной скобке на той же линии говорить не буду, но просто сами посмотрите на свой код, например функции get(), в пастебине и попробуйте посмотреть в случае:
if ()
{
}
что было бы намного читабельнее.
Опять же прямой SQL-код внутри класса, что недопустимо, у клиента может быть другая база, другие таблицы (хотя бы названия даже, у многих с префиксами). То есть, ваше решение абсолютно не конфигурабельно, а следовательно не юзабельно.
И несколько других замечаний.
В целом, могу предположить, что вы недавно сели за пхп, и за ООП в частности. Слишком это в глаза бросается. Здесь много работы по рефакторингу и оптимизации, а кроме того, главный вопрос: а есть ли уже готовые решения? Попробуйте задать его себе, посмотреть, как у людей сделано.
И напоследок, зачем к имени класса добавлять {}? И так ясно, что это класс.
+1
присоединяюсь к комментарию выше и от себя хотел бы добавить следущее:
— нет смысла копировать объект подключения к ДБ, используйте сразу mysqlLayer::load()->doSmth()
— switch в set() и логику в add — под рефакторинг.
— нет смысла копировать объект подключения к ДБ, используйте сразу mysqlLayer::load()->doSmth()
— switch в set() и логику в add — под рефакторинг.
0
Тоесть изначально записывать объект подключения в переменную и использовать его потом в классе не нужно? (Если объект подключения — паттерн «Синглтон»).
По поводу switch в set():
Неправильно, то что я явно указал названия свойств вместо проверки на их существование?
По поводу switch в set():
Неправильно, то что я явно указал названия свойств вместо проверки на их существование?
0
неа. А проверка существования класса должна осуществляться на слое выше, чем этот.
по поводу остальных методов — напрягает большое количество выходов из них. Три выхода из свитча, что-т мне подсказывает, вполне можно было бы заменить на единственную проверку.
Да, вот еще на что обратил внимание — использование тройственной логики (true, false, null) там, где можно обойтись двойственной (true, false). Например, для fixedType.
рекомендую покурить маны на тему «рефакторинг».
по поводу остальных методов — напрягает большое количество выходов из них. Три выхода из свитча, что-т мне подсказывает, вполне можно было бы заменить на единственную проверку.
Да, вот еще на что обратил внимание — использование тройственной логики (true, false, null) там, где можно обойтись двойственной (true, false). Например, для fixedType.
рекомендую покурить маны на тему «рефакторинг».
0
Кстати, автор знает про паттерн MVC? ООП, имхо, имеет смысл начинать с его освоения.
-1
Вот тут не соглашусь ;) MVC сам по себе очень спорный, с момента его рождения в Smalltalk-е прошло очень много времени, и те реализации, которые есть сейчас очень и очень спорные.
А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
+1
А можно поподробней про спорность MVC? Меня интересует конкретно Ваше мнение.
0
Немного не так выразился, хотел сказать «с точки зрения начинаний в изучении ООП данный паттерн очень спорный».
Считаю, что MVC это несколько другой уровень абстракции. Например, вот в этой замечательной книжке:
en.wikipedia.org/wiki/Design_Patterns_(book)
данного паттерна нет, так как он отражает архитектуру всего приложения, а это уже далеко не уровень «двух-трёх классов». Например, каждый компонент из MVC может быть реализован посредством нескольких паттернов проектирования. К тому же, общих устоявшихся подходов к реализации паттерна нет, каждый понимает его по своему, даже в рамках, казалось бы, строгого описания. В каждом приложении паттерн MVC реализуется по своему, потому изучать его в начале нет смысла, для начала стоит освоить Буча, Гамму и компанию, а потом уже задумываться об остальном.
Считаю, что MVC это несколько другой уровень абстракции. Например, вот в этой замечательной книжке:
en.wikipedia.org/wiki/Design_Patterns_(book)
данного паттерна нет, так как он отражает архитектуру всего приложения, а это уже далеко не уровень «двух-трёх классов». Например, каждый компонент из MVC может быть реализован посредством нескольких паттернов проектирования. К тому же, общих устоявшихся подходов к реализации паттерна нет, каждый понимает его по своему, даже в рамках, казалось бы, строгого описания. В каждом приложении паттерн MVC реализуется по своему, потому изучать его в начале нет смысла, для начала стоит освоить Буча, Гамму и компанию, а потом уже задумываться об остальном.
0
Если я правильно понял вашу позицию, то Вы предлагаете общую подготовку знаний перед работой с ООП, в частности, подробное знакомство с популярными паттернами. Со своей же стороны я предлагаю автору начать с MVC что бы он сам смог прийти к ним через понимание слоистости приложения. Снизу вверх и сверху вниз, мы говорим об одном, но о разных направлениях изучения. Какое выбрать — пусть решает автор.
0
В современном мире, MVC это догма. Такая же как ОтецСынСвятойдух в христианстве. ;)
Техника расщепления приложения на слои описана у Фаулера в Patterns of Enterprise Application Architecture. В двухслойной архитектуре слои называются Client и Server. В трехслойной архитектуре — Presentation, DomainLogic и DataSource. Слоев может быть и больше. Дело не в названиях и их количестве. Суть в том, что на практике бывает полезно, сильно сцепленные куски логики приложения, обосабливать друг от друга (образуя между ними слабые коммуникационные связи). Например, хорошо отделять DomainLogic (логику, ради которой и создается система ⓒ), от прочего, неизбежно сопутствующего барахла, — UI и внешних API приложения с одной стороны, и источников/хранилищ данных с другой.
В большей части интерпретаций, MVC это принцип, который используется для анализа задач, связанных с созданием инструментов пользователя (Tools). С точки зрения MVC, все интересующие программные артефакты рассматриваются в аспекте представлений и их преобразований из одного в другое. View отражает представление некоторой артефакта, в терминах пользовательской ментальной модели, Model — представление в терминах программной модели, а Controller — представление в терминах управления. Например, инструмент для рейтингования этого коммента, с точки зрения MVC может рассматриваться следующим образом. Для юзера это изображение двух изумительных квадратиков: красного и зеленого — которые символизируют критику и одобрение. Для хабрасервера это строка из набора {'minus','plus'}. Для браузера — два мышекликабельных объекта.
И то и другое, в общем-то, являются самостоятельными концепциями, и не зависят от ООП.
Техника расщепления приложения на слои описана у Фаулера в Patterns of Enterprise Application Architecture. В двухслойной архитектуре слои называются Client и Server. В трехслойной архитектуре — Presentation, DomainLogic и DataSource. Слоев может быть и больше. Дело не в названиях и их количестве. Суть в том, что на практике бывает полезно, сильно сцепленные куски логики приложения, обосабливать друг от друга (образуя между ними слабые коммуникационные связи). Например, хорошо отделять DomainLogic (логику, ради которой и создается система ⓒ), от прочего, неизбежно сопутствующего барахла, — UI и внешних API приложения с одной стороны, и источников/хранилищ данных с другой.
В большей части интерпретаций, MVC это принцип, который используется для анализа задач, связанных с созданием инструментов пользователя (Tools). С точки зрения MVC, все интересующие программные артефакты рассматриваются в аспекте представлений и их преобразований из одного в другое. View отражает представление некоторой артефакта, в терминах пользовательской ментальной модели, Model — представление в терминах программной модели, а Controller — представление в терминах управления. Например, инструмент для рейтингования этого коммента, с точки зрения MVC может рассматриваться следующим образом. Для юзера это изображение двух изумительных квадратиков: красного и зеленого — которые символизируют критику и одобрение. Для хабрасервера это строка из набора {'minus','plus'}. Для браузера — два мышекликабельных объекта.
И то и другое, в общем-то, являются самостоятельными концепциями, и не зависят от ООП.
+1
> А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
Нет, нет и еще раз нет. ООП надо начинать с SICP, а затем уже переходить к Бучу.
Нет, нет и еще раз нет. ООП надо начинать с SICP, а затем уже переходить к Бучу.
0
Чтобы знающий человек сразу знал, что реализован синглтон, метод ::load() стоило бы назвать ::getInstance()
0
знающий человек определит синглтон по сигнатуре, и без разницы как он называется. Naming conventions всякие разные бывают.
0
Изначально я вообще его хотел назвать не load а l, для сокращения при статическом вызове events::l()->add() например.
Поменяю на getInstance()
Поменяю на getInstance()
0
лучше откажись от statefull'ности. Вызывай сразу: Event::add().
0
Насколько я знаю, если я буду использовать только статические вызовы метода, то класс events не сможет сохранить все сообщения и вывести их в необходимом участке приложения при вызове метода get().
0
$this->e->setTable(array( 1 => 'Необходимый для обработки изображения файл не найден', 2 => 'Файл не является картинкой', 3 => 'Некорректный формат картинки (должен быть jpeg, gif или png). Файл имеет недопустимый формат', 4 => 'Не найдена необходимая функция обработки изображения', 5 => 'Необходимый ресурс не найден. Необходимо загрузить картинку с помощью функции load()', 6 => 'Некорректные размеры для изменения размера картинки', 7 => 'Некорректные исходные размеры картинки', 8 => 'Некорректные установки для пропорционального изменения картинки. Возможные варианты (height, width, auto). Выбранный вариант' ), 'error');
Все числа, отличные от 0 или 1, для программиста являются «магическими», такой подход кодирования я бы назван не желательным.
Чтобы понять, что делает этот код, в котором есть такие фрагменты:
<?php $this->e->add(6, 'highest:' . $highest . 'px'); ?>
Нужно постоянно держать перед глазами вышеприведенный код или листочек с кодами, что не способствует пониманию кода.В вашем случае я бы закодировал ошибки в стиле:
$this->e->setTable(array( 'file_not_found' => 'Необходимый для обработки изображения файл не найден', 'not_an_image' => 'Файл не является картинкой', ....
+1
Буду говорить исключительно о своих личных впечатлениях.
1. Имя класса следует писать с большой буквы в стиле CamelCase. Ввиду отсутствия (на данный момент) неймспейсов, хорошо бы было добавить к именам классов префикс, например, MySuperLib_Events.
2. Перечислять в «шапке» класса сигнатуры методов, наверное, не стоит. Для этого есть аутлайнер в среде разработки. Остальные поля неплохо бы прокомментировать.
3. Singleton. Без гласной на конце.
4. Я предпочитаю функцию is_null конструкции === null.
5. В методе load класса events наблюдается жесткая связка класса с неким классом mysqlLayer по имени. Вместо этого можно использовать метод-сеттер, в который передавать объект, реализующий некий интерфейс.
6. Метод load может вернуть false, хотя в докблоке заявлено, что возвращаемое значение — self (что тоже, строго говоря, некорректно; в этом случае, насколько я понимаю, придется писать имя класса).
7. Метод get класса events нечитаем. По-моему, ему нужна декомпозиция и правильное форматирование.
В общем, извините за такие придирки. По сути к проектированию классов только пятую можно отнести. Тем не менее, надеюсь они окажутся чем-то полезными.
Вообще, из кода класса довольно трудно понять, что он делает, и имена методов не подсказывают их назначения.
1. Имя класса следует писать с большой буквы в стиле CamelCase. Ввиду отсутствия (на данный момент) неймспейсов, хорошо бы было добавить к именам классов префикс, например, MySuperLib_Events.
2. Перечислять в «шапке» класса сигнатуры методов, наверное, не стоит. Для этого есть аутлайнер в среде разработки. Остальные поля неплохо бы прокомментировать.
3. Singleton. Без гласной на конце.
4. Я предпочитаю функцию is_null конструкции === null.
5. В методе load класса events наблюдается жесткая связка класса с неким классом mysqlLayer по имени. Вместо этого можно использовать метод-сеттер, в который передавать объект, реализующий некий интерфейс.
6. Метод load может вернуть false, хотя в докблоке заявлено, что возвращаемое значение — self (что тоже, строго говоря, некорректно; в этом случае, насколько я понимаю, придется писать имя класса).
7. Метод get класса events нечитаем. По-моему, ему нужна декомпозиция и правильное форматирование.
В общем, извините за такие придирки. По сути к проектированию классов только пятую можно отнести. Тем не менее, надеюсь они окажутся чем-то полезными.
Вообще, из кода класса довольно трудно понять, что он делает, и имена методов не подсказывают их назначения.
0
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Публикации
Изменить настройки темы
Проектирование ООП классов (php) — линч