Comments 23
Что плохо в коде:
Груда кода в глобальном namespace. Зачем?
Не нужен флаг isIE, нужно проверять возможности, которые требуются, а не браузер.
Местами как-то странно используется JS (например, чтобы поместить элемент в начало массива можно использовать unshift).
Более подробно не смотрел.
Груда кода в глобальном namespace. Зачем?
Не нужен флаг isIE, нужно проверять возможности, которые требуются, а не браузер.
Местами как-то странно используется JS (например, чтобы поместить элемент в начало массива можно использовать unshift).
Более подробно не смотрел.
Об этом, я, конечно, подумал, прежде чем дать именно такой код. Подумал вот что: не стоит переусердствовать в применении принципов (что сам по себе тоже принцип). Если код используется на данной странице и только на ней — что вы пишете? Глобальную переменную, не задумываясь о совместимости. Библиотеки и переносимые процедуры уже сами подумали о том, как уйти из [[Global Scope]]. Сделал это и я в объекте GameLife, но зачем переусердствовать в функциях на кнопках, если они только на этой странице? Поэтому всякие doXxxx..() — в глобальных переменных.
isIE — согласен, это по привычке, оно интуитивно понятно. Это как бы говорит: «а вот для IE поступим иначе». функция g() — это тоже, лёгкая замена $(). Есть в коде более серьёзный недостаток — функция initGL1(), на неё «не хватило сил» подойти объектно.
isIE — согласен, это по привычке, оно интуитивно понятно. Это как бы говорит: «а вот для IE поступим иначе». функция g() — это тоже, лёгкая замена $(). Есть в коде более серьёзный недостаток — функция initGL1(), на неё «не хватило сил» подойти объектно.
isIE — согласен, это по привычке, оно интуитивно понятно. Это как бы говорит: «а вот для IE поступим иначе»Ну вот ровно вот это — плохой подход. Думаю, все хаки, которые вы использовали, уже не нужны в IE9, а он скоро выйдет.
При моём весьма положительном отношении к Вам лично я совершенно согласен с shpaker, bolk и razon.
Ваш код далёк от идеального и перечислять можно очень долго. Ошибки есть по всем ступеням — идеология кода, html-верстка, кучу ненужных кусков, ошибочных кусков, непонятных кусков, огромное количество комментариев.
Идеология ООП не в том, чтобы вынести всё классы. ООП и классы это всё-равно, что война и танки. Можно сделать войну без танков, а танки могут быть и без войны.
Если хотите написать такой код, который можно было бы называть примером для новичков следует очень много исправить.
А теперь — малая толика ошибок из разных областей, которые я заметил беглым проглядыванием.
Проблемы с html-кодом:
Многословный заголовок страницы. Стандарт html5 позволяет и рекомендует намного более изящную запись, не соблюдены отступы:
Заменяем на
Кучу нарушений идеи ненавязчивого JavaScript.
Кое-где забыли скобки:
Фактически, хотя вы и объявили доктайп xHTML, но у вас "6 Errors, 22 warning(s)". А еще кучу семантических и стилистических ошибок.
Проблемы с javascript-кодом:
Тут можно критиковать почти каждую строчку. Я начну с топика.
Это далеко не лучшие традиции ООП и такой стиль никак не рекомендуемый для новичка.. Ниже аргументирую почему.
Нет, эффект размножения есть даже без наследования — а при обычном создании экземпляра класса. Но в данном случае создаётся не так много экземпляров, потому оно не играет критической роли. Другое дело, что вы утверждаете, что неокрепшие умы сочтут это хорошим стилем и правильной реализацией. Потому даже(тем-более!) в маленьких демонстративных примерах нельзя использовать такой способ.
Тем более, что из-за его использования у вас некрасиво смешались две области видимости, а в купе с однобуквенным наименованием переменных сделало код совершенно непонятным.
Зачем вы меняете содержимое объекта, который передается аргументом? А если мне он будет необходим дальше и именно с значениями null? А вы взяли и поменяли мнее их.
Вы совершенно не соблюдаете стиль.
Почему
Фигурные скобки совершенно не имеют закономерности расстановки. Кое-где они стоят в одну линию с условием, кое-где в следующей, кое-где развернутые, кое-где отсутствуют. Бардак, в общем.
Названия переменных — это просто кошмар. Однобуквенный хаос, который понятен только автору. При просмотре кода возникает впечатление, что он пропущен через обфускатор.
То, что вы сделали с условиями в
Если бы вы написали нормальный код, который потом пропрофилировали, то поняли, что смысла выносить ифы нету совершенно.
Ошибка в плане скорости у вас не в месторасположении условий, а в архитектуре. Я сейчас делаю сверхтяжёлый код, с попиксельной отрисовкой канвы размером 480*225, кучей матана, риалтайм-ресайзом изображений и т.п., который, при этом, умудряется более-менее выдавать 20-30 фпс. Но там куда больше вычислений чем в вашем коде. В реализации «жизни» всё должно просто летать.
Что это за кромешный ужас? oId впонле достаточно для уникальной идентификации элемента, зачем замедлять получение элемента, указывая проверку на принадлежность к классу?
Что случится в функции
Почему вы не закешировали результат
Почему кое-где ссылки на dom-объекты передаются в GameLife аргументами, а кое-где — берутся из «Магических строк» прям внутри метода
Ад внутри условия
Вообще названия переменных — приводят в ужас. Перечислю те, что есть в области видимости
И это только те, которые в локальной области видимости. При этом
Этот код не то что далёк от ООП — он вообще далек от хорошего кода. И то, что вы используете методы не делает его ООП.
Без слёз на него смотреть тяжело. И это и есть пример того самого ада, в который можно попасть при поддержке проектов на javascript.
Заключение
Если ничего не помешает, то, под влиянием от вашей статьи, я таки опубликую вариант «жизнь» на javascript best-practice кодом и подробным объяснением.
Если нужна помощь, можете написать мне в личку, обсудим.
Ваш код далёк от идеального и перечислять можно очень долго. Ошибки есть по всем ступеням — идеология кода, html-верстка, кучу ненужных кусков, ошибочных кусков, непонятных кусков, огромное количество комментариев.
Идеология ООП не в том, чтобы вынести всё классы. ООП и классы это всё-равно, что война и танки. Можно сделать войну без танков, а танки могут быть и без войны.
Если хотите написать такой код, который можно было бы называть примером для новичков следует очень много исправить.
А теперь — малая толика ошибок из разных областей, которые я заметил беглым проглядыванием.
Проблемы с html-кодом:
Многословный заголовок страницы. Стандарт html5 позволяет и рекомендует намного более изящную запись, не соблюдены отступы:
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"><html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
Заменяем на
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
Кучу нарушений идеи ненавязчивого JavaScript.
Кое-где забыли скобки:
<div id=dGL></div>
Фактически, хотя вы и объявили доктайп xHTML, но у вас "6 Errors, 22 warning(s)". А еще кучу семантических и стилистических ошибок.
Проблемы с javascript-кодом:
Тут можно критиковать почти каждую строчку. Я начну с топика.
Давайте даже такую простую задачу оформим в лучших традициях ООП
Это далеко не лучшие традиции ООП и такой стиль никак не рекомендуемый для новичка.. Ниже аргументирую почему.
Ниже будет скрипт с теми самыми перегруженными методами, которые, если бы применялись в наследовании, дали бы тот самый не рекомендуемый в последней статье эффект размножения функций и траты памяти.
Нет, эффект размножения есть даже без наследования — а при обычном создании экземпляра класса. Но в данном случае создаётся не так много экземпляров, потому оно не играет критической роли. Другое дело, что вы утверждаете, что неокрепшие умы сочтут это хорошим стилем и правильной реализацией. Потому даже(тем-более!) в маленьких демонстративных примерах нельзя использовать такой способ.
Тем более, что из-за его использования у вас некрасиво смешались две области видимости, а в купе с однобуквенным наименованием переменных сделало код совершенно непонятным.
if(conf.w ==null) conf.w = 402; //параметры, не будут жить как замыкания
if(conf.h ==null) conf.h = 321;
if(conf.o ==null) conf.o = g('dbGL1');
Зачем вы меняете содержимое объекта, который передается аргументом? А если мне он будет необходим дальше и именно с значениями null? А вы взяли и поменяли мнее их.
Вы совершенно не соблюдаете стиль.
Почему
this.cvs и this.gen объявлены с this, а w и h — с var?Фигурные скобки совершенно не имеют закономерности расстановки. Кое-где они стоят в одну линию с условием, кое-где в следующей, кое-где развернутые, кое-где отсутствуют. Бардак, в общем.
Названия переменных — это просто кошмар. Однобуквенный хаос, который понятен только автору. При просмотре кода возникает впечатление, что он пропущен через обфускатор.
То, что вы сделали с условиями в
this.show — это не оптимизация, а кромешный ужас. Их надо было схлопнуть и ни в коем случае не оптимизировать. Тормозит явно не банальный «иф». Вы, блин, arc в цикле делаете, и при этом выносите if, увеличивая код в 3-4 раза! А полученный результат от этого — ускорение в 0.01%. Это если повезет. Если бы вы написали нормальный код, который потом пропрофилировали, то поняли, что смысла выносить ифы нету совершенно.
Ошибка в плане скорости у вас не в месторасположении условий, а в архитектуре. Я сейчас делаю сверхтяжёлый код, с попиксельной отрисовкой канвы размером 480*225, кучей матана, риалтайм-ресайзом изображений и т.п., который, при этом, умудряется более-менее выдавать 20-30 фпс. Но там куда больше вычислений чем в вашем коде. В реализации «жизни» всё должно просто летать.
if(!isIE)
o.querySelector('.gen #' + oId).innerHTML = val;
else
g(oId).innerHTML = val;
Что это за кромешный ужас? oId впонле достаточно для уникальной идентификации элемента, зачем замедлять получение элемента, указывая проверку на принадлежность к классу?
Что случится в функции
this.initCvs, если отсутствует метод querySelector?Почему вы не закешировали результат
document.getElementsByName('rule') в this.checkRuleПочему кое-где ссылки на dom-объекты передаются в GameLife аргументами, а кое-где — берутся из «Магических строк» прям внутри метода
Ад внутри условия
if(mthd =='pre'){ остался для меня сущей загадкой. А перечитал его раз 5, но так и не понял, зачем он нужен. Между прочим, тут видно один из недостатков перегруженного конструктора — непонятно к какой области видимости относится какая переменная. Долго искал, где взялась oВообще названия переменных — приводят в ужас. Перечислю те, что есть в области видимости
this.showmthd, o, s, q, l, pi2, z2, z21, z3, z6, grid, rect, x, y
conf, o, GL, w, h, z, a, iStop, iStart, okrИ это только те, которые в локальной области видимости. При этом
z21 = z/2.3 (где логика?), q — канвас-контекст, а okr — сокращение от русского слова «окружение».Этот код не то что далёк от ООП — он вообще далек от хорошего кода. И то, что вы используете методы не делает его ООП.
Без слёз на него смотреть тяжело. И это и есть пример того самого ада, в который можно попасть при поддержке проектов на javascript.
Заключение
Если ничего не помешает, то, под влиянием от вашей статьи, я таки опубликую вариант «жизнь» на javascript best-practice кодом и подробным объяснением.
Если нужна помощь, можете написать мне в личку, обсудим.
спойлеров на хабре нетСпойлер — это информация. То, что вы называете спойлером, можно назвать «катом» (cut), например.
UFO just landed and posted this here
Дни «Жизни» на Хабре! :)
10k.aneventapart.com/Uploads/344/index.html#0,1;1,1;1,2;2,0;2,1
Сколько стало интересных статей на Хабре.
Вы лучше бы чем в пятый раз объяснять правила жизни на хабре, алгоритмы свои поописывали, а то лично мне такие строки:
или такие условия
Как-то по индуски )) Без обид конечно код работает нормально, но вот исходники иногда заставляют улыбаться ))
GL.cvs.width = GL.cvs.width;
или такие условия
if(!GL.rect){
if(...){
…
}else{
…
}else{
…
}else{
...}
}
Как-то по индуски )) Без обид конечно код работает нормально, но вот исходники иногда заставляют улыбаться ))
Привет! Спасибо.
Ради алгоритмов и их обсуждения как раз было написано. И всему есть своё объяснение. (Думаю, каждый найдёт образцы подобного кода у себя.)
> GL.cvs.width = GL.cvs.width;
Почитайте (согласен, тут мне надо было написать комментарий) документацию к canvas, и будет ясно, что это далеко не бессмысленная строчка — это способ очистки полотна (!).
Более явного способа очистки почему-то не наблюдается. Было бы лучше, конечно — что-то типа canvas.clear();.
Условия (при выводе канваса) — тоже очень нужная в данном случае вещь (это мне показалось очевидным, не стал комментировать в коде). Если бы я слепил их все в один цикл, а условия загнал внутрь, то цикл медленнее бы работал. Здесь — компромисс объёма кода и скорости выполнения.
Ради алгоритмов и их обсуждения как раз было написано. И всему есть своё объяснение. (Думаю, каждый найдёт образцы подобного кода у себя.)
> GL.cvs.width = GL.cvs.width;
Почитайте (согласен, тут мне надо было написать комментарий) документацию к canvas, и будет ясно, что это далеко не бессмысленная строчка — это способ очистки полотна (!).
> canvas.width = canvas.width; // clears the canvas
Более явного способа очистки почему-то не наблюдается. Было бы лучше, конечно — что-то типа canvas.clear();.
Условия (при выводе канваса) — тоже очень нужная в данном случае вещь (это мне показалось очевидным, не стал комментировать в коде). Если бы я слепил их все в один цикл, а условия загнал внутрь, то цикл медленнее бы работал. Здесь — компромисс объёма кода и скорости выполнения.
вот оно как
Я просто на канве обычно всё перерисовываю и поэтому как-то не задумывался о очистке её вообще, хотя можно было бы clearRect, но да такой как у вас способ оправданней наверное
Я просто на канве обычно всё перерисовываю и поэтому как-то не задумывался о очистке её вообще, хотя можно было бы clearRect, но да такой как у вас способ оправданней наверное
Добавил в работающий код (и в статью) вариацию правил — Day & Night. Описана в ссылках [7] в Википедии (англ.). Довольно интересное поведение.
у меня кстати тоже есть Жизнь на канвасе писанная, я как разгребусь с делами выложу. Я там правда немного отсебятины в правила вносил ))
Давайте даже такую простую задачу оформим в лучших традициях ООП
Посмотрел код. Позвольте спросить, где в нем ООП?
Про кучу дублирования я вообще молчу
Про дублирование там, где надо максимально ускорить циклы, я уже писал в комментариях. ООП — в определении объекта — игрового поля и методов работы с ним. Если у Вас возникли более конструктивные предложения — пожалуйста, покажите, как надо делать. Иначе это не критика, а разговор ни о чём. С уважением…
Вместо того, чтобы издеваться над массивом(с постоянным перемещением в начало) можно было бы сделать два массива, которые менять местами. Один используется как источник для наполнения данными второго.
Но медлительность убивает. Все должно работать намного быстрее при должном подходе.
Но медлительность убивает. Все должно работать намного быстрее при должном подходе.
Sign up to leave a comment.
Javascript и canvas в игре «Жизнь» Джона Конвея