Креш-тест: оценка уровня написания JavaScript-кода

Иван Деменков пишет:
— Оцените, пожалуйста, уровень написания кода — demenkov.dp.ua/dou.
Что хорошо, что плохо, и в каком направлении сосредоточить усилия по повышению своей квалификации?

Я почитал код и в целом, нужно сказать, всë довольно неплохо. Т.е. все минусы, которые я пишу ниже, это не какие-то глобальные проблемы, а разные мелочи.

Очевидно, что по трём таким примерам оценить уровень мне сложно, но если бы я такое увидел на собеседовании, я бы скорее всего человека взял ;)

Итак, по минусам:

  • Это не имеет отношения к именно скиллу программирования, но всë же. Стилистика хромает. То есть пробелы, то их нет: в перечислении аргументов функции, после слов if/for, при присвоении, и т.п. Это раздражает, если честно, несколько тяжело читать конструкции «for(i=0;i<bodychilds.length;i++)». Ширина строк — до бесконечности. Это уже вопрос такой, мы у себя держит не шире 80 символов, кто-то ставит 120, но всë равно, нет смысла писать очень много в одну строку. В первом файле (остальные не проверял на это) смешаны одновременно табы и пробелы для отступов.
  • Строки, как управляющие элементы — это неправильно. В них можно ошибиться и никогда этого не узнаешь. Конкретное место — в первом примере функция manipulateBlock. Если так хочется открывать/закрывать одной функцией, можно было сделать параметр-boolean, или что-нибудь такое. Или chartMode. Я бы выделил из drawData какой-то хелпер и сделал бы две функции, одну для рисования линий, другую — для гистограмм.
  • Тот же manipulateBlock. Цикл for идëт по всем дивам, несмотря на то, что нужный существует всего один. Не проще ли было сделать функцию getBlock, которая берëт айдишник и по нему находит и возвращает блок? Тогда и смысла в одной функции на сохранение и удаление не остаëтся. Более того, можно было id ложить не в атрибут idblock (который, кстати, не существует, для них рекомендуют использовать атрибуты data-*), а просто в id — и тогда getElementById, и дело в шляпе.
  • Один хендлер на все ивенты — спорное решение, и более того, onclick? addEventListener же.
  • Маленькая (совсем) ошибка проектирования. Во втором примере getData, если это будет аяксовая функция, не сможет ничего возвращать. Ей нужен коллбэк.
  • Опять поиск таблицы (в 3 примере) с помощью цикла. getElementsByTagName("table")[0]?
  • Не понимаю, зачем постоянно использовать worker.что-то, если можно this.что-то?

Фух, закончил вроде. В целом всë клëво, я довольно придирчивый, мне кажется. По крайней мере, у нас в команде я очевидно самый придирчивый, так что не напрягайся сильно.

Мой вариант решения задачи № 1

Он работает идентично оригинальному. Я не менял стили/HTML (практически), только заменил JavaScript.

Я выделил отдельно функции работы с меню, блоками, и просто общие хелперы. Это упрощает тестирование (можно тестировать каждый уровень функциональности отдельно), и понимание — легче понять, кто за что отвечает.

Например, уровень обработки блоков ничего не знает про меню и не в курсе про структуру DOM, меню спокойно себе тасует данные и почти не в курсе про блоки (ну, немного в курсе, конечно ;)).

Один из ивент-хендлеров делает немного больше, чем я хотел бы (аппендит блок к родительскому элементу), но при таком количестве кода я не вижу смысла про это много думать. Ну и в целом я постарался организовать/отформатировать код так, чтоб это было легко читать.

Можно было бы, кстати, завернуть блоки в объекты, и т. д., но большого смысла это не несëт. Тут не надо менеджить слишком сложные структуры и не предполагается развития кода, поэтому хранение данных в DOM вполне нормально работает.

Удачи!

P.S. И английский нужно импрувить, если надписи во втором примере — не опечатки/etc. О чëм ты знаешь, наверное.

Маєте важливу новину про українське ІТ? Розкажіть спільноті. Це анонімно.І підписуйтеся на Telegram-канал редакції DOU

👍НравитсяПонравилось0
В избранноеВ избранном0
Подписаться на автора
LinkedIn



Підписуйтесь: Soundcloud | Google Podcast | YouTube


11 комментариев

Подписаться на комментарииОтписаться от комментариев Комментарии могут оставлять только пользователи с подтвержденными аккаунтами.

Задача 1, в методе appendBlock
div = this.setBlock( parent, x, y, 'no name', 'no description' ); // задаем значения по умолчанию
создает глобальную переменную

Ой, не заметил комментарий ниже

По первому примеру(только его глянул) в добавок к коменту по ие :
>> div = setBlock(parent, x, y, ’no name’, ’no description’); // задаем значения по умолчанию
Не мешало — бы «var» добавить, ибо иначе попадает переменная в глобал скоуп.

А так — все неплохо.

Имхо в первом примере комментариев чуть более чем дофига. Отлично для документации в учебнике, но хреново для документации кода: выглядит в духе «a=a+2; //а вот тут мы в переменную а добавляем 2».

По-моему логичнее в нужном объеме документировать логический блок (если надо) и логические ветвления и элементы (опять же, если надо), но зато документировать ключевые функции/методы.

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

Что же касается именно документирования — это делается на уровне интерфейсов методов и объектов. Пояснять же реализацию комментариями не должно быть нужно.

Если появляются комментарии — это свидетельство неочевидных решений/кривого кода. Бывают исключения, но ооочень редко.

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

Александр, еще раз благодарю за «разбор полетов» и свой вариант выполнения примера, — мне как раз не хватает такого стиля в моей работе. Вот, теперь есть от чего отталкиваться. :) Английский — в работе, уже хожу на курсы

Заметил, что оба варианта первого примера выдают ошибки в Internet Explorer 8. В IE 9 и других браузерах все норм.

В коде Ивана, при первом клике на картинке выскакивает ошибка: Invalid argument

i.imgur.com/t53fs.png

В коде Александра, при загрузке страницы: Error: Object doesn’t support property or method ’addEventListener’

i.imgur.com/W4dDd.png

Еще раз подчеркиваю, что эти проблемы можно воспроизвести только в IE8/IE7

Ваш тестировщик

Ха-ха, найс, давно не возился с этим. Пришлось даже в гугле поискать, чтоб вспомнить, что там attachEvent. :)

jquery-like framework?

Are is it shitcoding?...

это круто. Утром в понедельник не сразу въедешь, что эта фраза означает :-)

Подписаться на комментарии