На сколько правильно написан мой код?

Question

[Даниил Сухих]

Недавно начал учить js, до этого знал только jquery.
Написал код который открывает и закрывает мобильное меню. А так же закрывает его при нажатии на пункты меню. На случай если пункт меню будет якорем.
Мне кажется что мой код слишком большой и использован не верный подход.
Если это так, скажите что мне исправить.

var mobileButton = document.querySelector('.mobileButton');
var headerNavMenu = document.querySelector('.headerNav > ul');
var closeMobileMenu = document.querySelector('.closeMobileMenu');
var menuList = document.querySelectorAll('.headerNav > ul > li');

mobileButton.onclick = menuToggle;
closeMobileMenu.onclick = menuToggle;
for (var i = menuList.length - 1; i >= 0; i--) {
	menuList[i].onclick = menuToggle;
}

function menuToggle() {
	headerNavMenu.style.display = (headerNavMenu.style.display == 'none' || headerNavMenu.style.display == '') ? 'flex' : 'none';
}

Comments

[Шохрух Шаймардонов]

Используйте принцип DRY. Если одну и ту же часть кода повторяете несколько раз, то оберните его в функцию и вызывайте в нужный момент.

Это совет на все времена

[beDenz]

for (var i = menuList.length - 1; i >= 0; i--) {
  menuList[i].onclick = menuToggle;
}

forEach будет красивее

headerNavMenu.style.display = (headerNavMenu.style.display == 'none' || headerNavMenu.style.display == '') ? 'flex' : 'none';

Субъективно конечно, но такие вещи лучше делать сменой классов

[Пашенька]

beDenz, а ничего что for в сто раз быстрее форича?

Самый главный совет здесь должен быть: написать нормальные селекторы и использовать контекст, а не сёрчить по всему документу.

[beDenz]

like-a-boss, а ракета быстрей автомобиля, и что. Нужно анализировать ситуацию, и выбирать более подходящие варианты.

[Пашенька]

beDenz, вот только весь ваш анализ основывается на «красивее».

[WapSter]

like-a-boss, в данном случае ни на что не повлияет. Работая с огромным количеством данных да стоит использовать for.

[beDenz]

like-a-boss, писать код нужно для людей

Answer 1

[Сергей Соколов]

Можно querySelectorAll() сразу по нескольким селекторам.

const headerNavMenu = document.querySelector('.headerNav > ul');

const menuToggle = () => {
  const value = headerNavMenu.style.display;
  headerNavMenu.style.display = (value == 'none' || value == '') ? 'flex' : 'none';
}  

[...document.querySelectorAll('.mobileButton, .closeMobileMenu, .headerNav > ul > li')]
.forEach(el => el.addEvenetListener('click', menuToggle));

Comments

[beDenz]

[...document.querySelectorAll('.mobileButton, .closeMobileMenu, .headerNav > ul > li')]

у нас в конторе за такое бы просто распяли бы, потому что не кроссбраузерно

[WapSter]

beDenz, про babel не слышали? Грустно и печально работать в таком месте.

[beDenz]

WapSter, слышали конечно, но категорически против его использования

[Никита]

beDenz, и чем же это обосновано? Возьмите любую крупную компанию - они работают с babel

[Сергей Соколов]

beDenz, зато у вас, наверное, климат тёплый.
Контекст вопроса не оговаривает требований к версиям браузеров и религий. Поэтому ваш дублированный комментарий неуместен.
"Удоли"

[beDenz]

Никита Пурыга, аргументируется тем, что зачем писать код на es6 если в конечном итоге он преобразуется в es5. можно сразу же писать на нем.

[Даниил Сухих]

...document
Зачем эти точки?

[Alex]

danchikraw, https://developer.mozilla.org/uk/docs/Web/JavaScri...

[Alex]

beDenz,

аргументируется тем, что зачем писать код на es6 если в конечном итоге он преобразуется в es5. можно сразу же писать на нем

Ну, это как минимум обосновано прибылью. Код написанный в более современном стандарте меньше, легче читается, проще поддерживается. А значит, исправление какого-то бага, или внедрение новой фичи происходит быстрее и с меньшими усилиями. Грубо говоря, бизнес получает тот же результат оплачивая меньше человеко-часов работы.

И это не говоря уже о производительности. Очень часто применяемая практика — сборка бабелем или ещё чем не одной а нескольких сборок. Скажем для IE — сборка в ES5, для вечно зеленых браузеров ES2018, для остальных — ES6. Так как код в новом стандарте в большинстве своём меньше — вы получаете экономию на трафике, а пользователь увеличивает скорость загрузки. + Браузер может по разному обрабатывать и применять внутренний оптимизации для кода в новом стандарте, что так же увеличивает скорость работы.

Но, я всего лишь какой-то человек в интернете. Что я могу понимать?

UPD
Зачем писать на C++ если в конечном итоге он преобразуется в байткод? можно сразу же писать на нем

[Сергей Соколов]

danchikraw, это один из способов сделать из массивоподобного результата вызова document.querySelectorAll() настоящий массив — потому, что понадобится его метод forEach()
Это ES6 синтакс, в старых браузерах не поддерживается. Ссылку на доку уже дали в комментарии выше.

[Владимир Солнцев]

beDenz, зачем писать на es5, если в конечном итоге браузерам придется поддерживать es6, а сейчас есть бабель и другие?

Разница между вашей и моей аргументацией лишь в направленности. Ваша - в прошлая, моя - в будущее)
Тут еще можно написать много букаф о удобстве, наглядности, и новых возможностях, но зачем.

[Даниил Сухих]

Сергей Соколов, правильно я понимаю, он набор элементов, преобразует в массив?

[beDenz]

danchikraw, да, так же как и

[].slice.call(document.querySelectorAll('.mobileButton, .closeMobileMenu, .headerNav > ul > li'));

Answer 2

[Владимир Солнцев]

Правильно - очень неопределенное понятие. Если смотреть буквально, то если ваш код работает, он написан правильно. Был бы написан не правильно - не работал бы.

Если вас интересуют варианты рефакторинга, то можно сделать код более универсальным.
У вас сейчас все привязано железно привязано к конкретным элементам, захотите, например, сделать тогглер для другого меню и придется опять писать этот код. А это излишне, ведь логика будет повторятся: при клике на кнопку, показать или скрыть какой-то элемент.
Можете попробовать разделить логику и элементы. Начать можно с того, чтобы передавать в menuToggle элемент аргументом. Либо, сделать меню отдельным независимым компонентом.
В общем можете переписать код в функциональном стиле или в ООП.

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

Про цикл уже выше писали, соглашусь: вместо императивного for, мне больше нравится декларативные встроенные методы.