Comments 31
Ресайз самый обычный, лучше бы swtich'om сделали. вместо^
Извините, что с 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)
Прочитав статью habrahabr.ru/blogs/development/51518/ про исключения, я для себя сделал вывод, что в избытке использовать исключения для вывода сообщений пользователю / записью в базу -> плохо.
Можно поподробнее про enum?
Можно поподробнее про enum?
Постараюсь. Не совсем уверен, уместно ли это в комментах — по вашему желанию можем переместиться в личную переписку.
Кратко, 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.
Есть альтернатива enum'у в ряде случаев. Использовать методы несущие в своих названиях значения констант. Для логов, например: void saveSuccessMsg(string $msg), таким образом наружу торчат понятные имена простых функций, а все тонкости флажков и состояний — спрятаны в классе.
Синглтон тут в тему, ибо не нужны другие объекты на основе этого класса. В принципе сообщение в лог должно быть stateless, то есть — не иметь свойств необходимых для хранения. (Если, например, не считается количество сообщений за одно обращение к коду). То есть можно написать просто класс с набором статических функций
Синглтон тут в тему, ибо не нужны другие объекты на основе этого класса. В принципе сообщение в лог должно быть stateless, то есть — не иметь свойств необходимых для хранения. (Если, например, не считается количество сообщений за одно обращение к коду). То есть можно написать просто класс с набором статических функций
Простите, второго класса не видел, но зачем целый класс, когда есть GD и один, фактически, метод для ресайза?
В эвентах вы смешали логику и представление. Кодирование конкретного вида (HTML-тегов) внутри класса это очень и очень плохо. Что бы клиенту вашего класса поменять внешний вид, ему прийдется лезть внутрь и менять. К тому же вы выводите данные непосредственно (echo), что явно непозволительно в большинстве случаев.
Так же общая нечитабельность кода, например, return-ы в разных местах функции, при сопровождении много времени теряется только на восстановление логики работы функции.
О фигурной скобке на той же линии говорить не буду, но просто сами посмотрите на свой код, например функции get(), в пастебине и попробуйте посмотреть в случае:
if ()
{
}
что было бы намного читабельнее.
Опять же прямой SQL-код внутри класса, что недопустимо, у клиента может быть другая база, другие таблицы (хотя бы названия даже, у многих с префиксами). То есть, ваше решение абсолютно не конфигурабельно, а следовательно не юзабельно.
И несколько других замечаний.
В целом, могу предположить, что вы недавно сели за пхп, и за ООП в частности. Слишком это в глаза бросается. Здесь много работы по рефакторингу и оптимизации, а кроме того, главный вопрос: а есть ли уже готовые решения? Попробуйте задать его себе, посмотреть, как у людей сделано.
И напоследок, зачем к имени класса добавлять {}? И так ясно, что это класс.
В эвентах вы смешали логику и представление. Кодирование конкретного вида (HTML-тегов) внутри класса это очень и очень плохо. Что бы клиенту вашего класса поменять внешний вид, ему прийдется лезть внутрь и менять. К тому же вы выводите данные непосредственно (echo), что явно непозволительно в большинстве случаев.
Так же общая нечитабельность кода, например, return-ы в разных местах функции, при сопровождении много времени теряется только на восстановление логики работы функции.
О фигурной скобке на той же линии говорить не буду, но просто сами посмотрите на свой код, например функции get(), в пастебине и попробуйте посмотреть в случае:
if ()
{
}
что было бы намного читабельнее.
Опять же прямой SQL-код внутри класса, что недопустимо, у клиента может быть другая база, другие таблицы (хотя бы названия даже, у многих с префиксами). То есть, ваше решение абсолютно не конфигурабельно, а следовательно не юзабельно.
И несколько других замечаний.
В целом, могу предположить, что вы недавно сели за пхп, и за ООП в частности. Слишком это в глаза бросается. Здесь много работы по рефакторингу и оптимизации, а кроме того, главный вопрос: а есть ли уже готовые решения? Попробуйте задать его себе, посмотреть, как у людей сделано.
И напоследок, зачем к имени класса добавлять {}? И так ясно, что это класс.
присоединяюсь к комментарию выше и от себя хотел бы добавить следущее:
— нет смысла копировать объект подключения к ДБ, используйте сразу mysqlLayer::load()->doSmth()
— switch в set() и логику в add — под рефакторинг.
— нет смысла копировать объект подключения к ДБ, используйте сразу mysqlLayer::load()->doSmth()
— switch в set() и логику в add — под рефакторинг.
Тоесть изначально записывать объект подключения в переменную и использовать его потом в классе не нужно? (Если объект подключения — паттерн «Синглтон»).
По поводу switch в set():
Неправильно, то что я явно указал названия свойств вместо проверки на их существование?
По поводу switch в set():
Неправильно, то что я явно указал названия свойств вместо проверки на их существование?
неа. А проверка существования класса должна осуществляться на слое выше, чем этот.
по поводу остальных методов — напрягает большое количество выходов из них. Три выхода из свитча, что-т мне подсказывает, вполне можно было бы заменить на единственную проверку.
Да, вот еще на что обратил внимание — использование тройственной логики (true, false, null) там, где можно обойтись двойственной (true, false). Например, для fixedType.
рекомендую покурить маны на тему «рефакторинг».
по поводу остальных методов — напрягает большое количество выходов из них. Три выхода из свитча, что-т мне подсказывает, вполне можно было бы заменить на единственную проверку.
Да, вот еще на что обратил внимание — использование тройственной логики (true, false, null) там, где можно обойтись двойственной (true, false). Например, для fixedType.
рекомендую покурить маны на тему «рефакторинг».
Кстати, автор знает про паттерн MVC? ООП, имхо, имеет смысл начинать с его освоения.
Вот тут не соглашусь ;) MVC сам по себе очень спорный, с момента его рождения в Smalltalk-е прошло очень много времени, и те реализации, которые есть сейчас очень и очень спорные.
А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
А можно поподробней про спорность MVC? Меня интересует конкретно Ваше мнение.
Немного не так выразился, хотел сказать «с точки зрения начинаний в изучении ООП данный паттерн очень спорный».
Считаю, что MVC это несколько другой уровень абстракции. Например, вот в этой замечательной книжке:
en.wikipedia.org/wiki/Design_Patterns_(book)
данного паттерна нет, так как он отражает архитектуру всего приложения, а это уже далеко не уровень «двух-трёх классов». Например, каждый компонент из MVC может быть реализован посредством нескольких паттернов проектирования. К тому же, общих устоявшихся подходов к реализации паттерна нет, каждый понимает его по своему, даже в рамках, казалось бы, строгого описания. В каждом приложении паттерн MVC реализуется по своему, потому изучать его в начале нет смысла, для начала стоит освоить Буча, Гамму и компанию, а потом уже задумываться об остальном.
Считаю, что MVC это несколько другой уровень абстракции. Например, вот в этой замечательной книжке:
en.wikipedia.org/wiki/Design_Patterns_(book)
данного паттерна нет, так как он отражает архитектуру всего приложения, а это уже далеко не уровень «двух-трёх классов». Например, каждый компонент из MVC может быть реализован посредством нескольких паттернов проектирования. К тому же, общих устоявшихся подходов к реализации паттерна нет, каждый понимает его по своему, даже в рамках, казалось бы, строгого описания. В каждом приложении паттерн MVC реализуется по своему, потому изучать его в начале нет смысла, для начала стоит освоить Буча, Гамму и компанию, а потом уже задумываться об остальном.
Если я правильно понял вашу позицию, то Вы предлагаете общую подготовку знаний перед работой с ООП, в частности, подробное знакомство с популярными паттернами. Со своей же стороны я предлагаю автору начать с MVC что бы он сам смог прийти к ним через понимание слоистости приложения. Снизу вверх и сверху вниз, мы говорим об одном, но о разных направлениях изучения. Какое выбрать — пусть решает автор.
В современном мире, 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'}. Для браузера — два мышекликабельных объекта.
И то и другое, в общем-то, являются самостоятельными концепциями, и не зависят от ООП.
> А ООП лучше начинать изучать с Гради Буча ;-) Потом плавно переходить к Гамма и Ко.
Нет, нет и еще раз нет. ООП надо начинать с SICP, а затем уже переходить к Бучу.
Нет, нет и еще раз нет. ООП надо начинать с SICP, а затем уже переходить к Бучу.
Чтобы знающий человек сразу знал, что реализован синглтон, метод ::load() стоило бы назвать ::getInstance()
знающий человек определит синглтон по сигнатуре, и без разницы как он называется. Naming conventions всякие разные бывают.
Ну открыли вы кусок кода vim-ом, на удаленном сервере. Вы будете лезть в код класса, чтобы сигнатуру посмотреть?
чтобы узнать сигнатуру нет необходимости лезть в код класса.
Изначально я вообще его хотел назвать не load а l, для сокращения при статическом вызове events::l()->add() например.
Поменяю на getInstance()
Поменяю на getInstance()
$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. Имя класса следует писать с большой буквы в стиле 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 нечитаем. По-моему, ему нужна декомпозиция и правильное форматирование.
В общем, извините за такие придирки. По сути к проектированию классов только пятую можно отнести. Тем не менее, надеюсь они окажутся чем-то полезными.
Вообще, из кода класса довольно трудно понять, что он делает, и имена методов не подсказывают их назначения.
Sign up to leave a comment.
Проектирование ООП классов (php) — линч