Задать вопрос
@evgeniyabutukhanova
javascript

Как усовершенствовать код и избавиться от множества глобальных переменных?

Данный код предназначен для формирования списка уникальных элементов массива, сделанного из двух других.
Тот факт, что списка два и нужно именно два списка преобразовывать в два разных массива. является частью задачи.
Этот код с задачей справляется, но в нём множество глобальных переменных, а это, как мне известно, не является хорошей практикой.
Подскажите, пожалуйста, как можно усовершенствовать его?

<head>
    <meta charset="utf-8">
    <title>Test</title>
    <style>
      div {
        width: 300px;
        margin: 10px;
        padding: 2px 5px;
        border: 1px solid black;   
      }
    </style>
  </head>
  <body id="body">
    <div>
      <ul id="ul1">
        <li>Любовь</li>
        <li>Надежда</li>
        <li>Вера</li>
      </ul>
    </div>
    <div>
      <ul id="ul2">
        <li>София</li>
        <li>Надежда</li>
        <li>Вера</li>
      </ul>
    </div>
    <button id="makeNewArray">Создать новый массив</button>
    <script type="text/javascript">
      function init() {
        var btn = document.getElementById('makeNewArray'),
            arrLi1 = ul1.getElementsByTagName('li'),
            arrLi2 = ul2.getElementsByTagName('li'),
            arr1 = [],
            arr2 = [],
            result = [];
        for (var i = 0; i < arrLi1.length; i++) {
          arr1.push(arrLi1[i].innerHTML);
        }
        for (var j = 0; j < arrLi2.length; j++) {
          arr2.push(arrLi2[j].innerHTML);
        }
        commonArr = arr1.concat(arr2);
        nextInput:
          for (var k = 0; k < commonArr.length; k++) {
            var str = commonArr[k];
            for (var l = 0; l < result.length; l++) {
            if (result[l] == str) continue nextInput;
            }
            result.push(str);
          }
        btn.onclick = function() {
          var div = document.createElement('div');
          div.innerHTML = result;
          body.appendChild(div);
        };
      };
      window.onload = init;
    </script>
  </body>
  • Вопрос задан
  • 652 просмотра
Комментировать
Подписаться 1 Оценить Комментировать
Решения вопроса 1
alexey-m-ukolov
@alexey-m-ukolov Куратор тега JavaScript
https://jsfiddle.net/koceg/66dd35hr/

В вашем коде, несмотря на то, что он небольшой, довольно много проблем. Давайте пойдем по порядку:

  1. Он в принципе не работает, потому что переменной body не существует. Нужно обращаться через document.body.

  2. У вас всё в одной функции. Отсюда и глобальные переменные (на самом деле они не глобальные, а ограничены этой функцией, но всё же) и дублирование одного и того же кода.

  3. Использование того, что в глобальной области видимости создаются переменные для каждого элемента с id (ul1, ul2) - плохая практика. Чуть раньше вы корректно воспользовались document.getElementById().

  4. Использование label в continue - приравнивается к goto и является уголовно наказуемым деянием.

  5. Вместо собственной реализации поиска нужно использовать стандартный метод indexOf, тогда и goto не понадобится.

  6. Из-за использования innerHTML, достаточно просто перенести закрывающий тег </li> на новую строку, чтобы элемент задублировался.

  7. Код срабатывает при старте страницы, а не тогда, когда он реально нужен. Это и само по себе проблема - зачем делать вычисления, которые неизвестно понадобятся ли, и не позволит изменить результат при изменении списков.

  8. Есть еще несколько мелочей, которые я не стал править, вроде установки обработчиков событий через .onclick, это будет домашнее задание.
Ответ написан
Комментировать
Комментировать
Пригласить эксперта
Ответы на вопрос 1
vawsan
@vawsan
Frontend Developer
Вот более правильный вариант, вам правда уже быстрее меня ответили.
<script type="text/javascript">
		(function () {
			function makeArray() {
				var arr1 = getArray("#ul1"),
					arr2 = getArray("#ul2");

				function getArray(selector) {
					if (selector)
						return Array.prototype.slice.apply(document.querySelector(selector).getElementsByTagName('li'))
							.map(function (curr) {
								return curr.innerHTML
							})
					return [];
				}

				for (var i = 0; len = arr2.length, i < len; i++) {
					if (arr1.indexOf(arr2[i]) < 0)
						arr1.push(arr2[i])
				}
				return arr1.join(", ");
			};

			document.getElementById('makeNewArray').onclick = function () {
				var div = document.createElement('div');
				div.innerHTML = makeArray();
				body.appendChild(div);
			};
		})();
	</script>

Но это легко понимаемый вариант с получением списков по отдельности.

Если же вам все равно сколько списков и не важно, откуда каждый элемент по отдельности, есть мегакороткий вариант:
<script type="text/javascript">
		(function () {
			function makeArray() {
				return Array.prototype.slice.apply(document.querySelectorAll("li"))
					.map(function (curr) {
						return curr.innerHTML
					})
					.filter(function (item, pos, self) {
						return self.indexOf(item) == pos;
					})
					.join(", ");
			};

			document.getElementById('makeNewArray').onclick = function () {
				var div = document.createElement('div');
				div.innerHTML = makeArray();
				body.appendChild(div);
			};
		})();
	</script>
Ответ написан
Комментировать
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы
Вакансии с Хабр Карьеры
JavaScript разработчик
SummerWeb • Ярославль
от 100 000 до 140 000 ₽
JavaScript разработчик
вАйТи
от 5 000 до 25 000 ₽
Senior Fullstack JavaScript Node.js + React
Simplenight inc.
от 4 000 до 6 500 $
Ещё вакансии
Заказы с Хабр Фриланса
Ищу специалиста по безопасности сайта
04 мая 2024, в 19:00
10000 руб./за проект
Развернуть опенсорсный проект Vocode на публичном сервисе Vercel
04 мая 2024, в 18:49
3000 руб./за проект
Разработать 3Д модель сверла в NX10
04 мая 2024, в 17:40
1000 руб./за проект
Ещё заказы