2
\$\begingroup\$

This code is made to generate a list of unique elements of the array made of the other two arrays. The fact that we need two lists to convert in two different array is part of the task. This code handles with the task, but there's a lot of global variables. As far as I know, it is not good practice. Tell me, please, how can I improve it?

<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>

|improve this question
\$\endgroup\$
  • 4
    \$\begingroup\$ You don't have any global variables, they all local in init function. \$\endgroup\$ – jcubic Dec 24 '15 at 11:25
  • 1
    \$\begingroup\$ Notice, that all the variables are used in init, you can declare commonArray and str in that function too. Unless they are used in some other script too. A common way is to enclose all the script within a single IIFE, that way you wouldn't have to use globals at all. \$\endgroup\$ – Teemu Dec 24 '15 at 11:32
2
\$\begingroup\$

As already noticed by previous comments, don't mind about global variables if they're declared by var inside of a function.
And you can do the same for commonArray and str, unless they're used elsewhere (obviously, I don't think so).

In the other hand, you can dramatically reduce your code!

First because you don't have to wait for onload event, since your <script> is located at the end of the <body>.
So you may replace this structure:

  function init() {
    var btn = document.getElementById('makeNewArray'),
    // ...computing result...
    btn.onclick = function() {
      var div = document.createElement('div');
      div.innerHTML = result;
      body.appendChild(div);
    };
  };
  window.onload = init;

by this one:

  document.getElementById('makeNewArray').onclick = function() {
    // ...computing result...
    var div = document.createElement('div');
    div.innerHTML = result;
    body.appendChild(div);
  };

Then in the computing result part, you can also work in a more light way, using less variables and less statements. This achieved essentially by not creating temporary containers, and directly populating (or not) the final result while iterating the source <ul>s. Looks like this:

<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">
document.getElementById('makeNewArray').onclick = function() {
  var div = document.createElement('div'),
      sources = [ul1, ul2],
      result = [];
  for (var i = 0, n = sources.length; i < n; i++) {
    var items = sources[i].getElementsByTagName('li');
    for (var j = 0, m = items.length; j < m; j++) {
      var str = items[j].innerHTML;
      if (result.indexOf(str) == -1) {
        result.push(str);
      }
    }
  }
  div.innerHTML = result;
  body.appendChild(div);
};
</script>
</body>

Here is a working example.

|improve this answer
\$\endgroup\$
0
\$\begingroup\$

You are using some variables (ul1 and ul2) like "magic" global variables. That is a sloppy IE behaviour that other browsers have copied, and is not recommended. (One reason is that it treats name and id attributes alike.) The fix for that problem is simple: use document.getElementById(…) instead.

Your code is odd, in that the computation occurs at load time; clicking the button merely outputs a copy of the pre-computed result. Is that what you intended?

I recommend splitting the function up into three:

  • Attaching the button's handler
  • A function that merges HTML lists
  • An event handler that calls the merging function with the appropriate lists and outputs the result

In the merge function, I suggest using a hash to detect duplicates. Otherwise, you would have an O(n) search for each item, for a total of O(n2).

window.addEventListener('load', function() {
    document.getElementById('makeNewArray').addEventListener('click', buttonClicked);
});

function buttonClicked() {
    var output = document.createElement('div');
    output.innerHTML = mergeLists([
        document.getElementById('ul1'),
        document.getElementById('ul2'),
    ]);
    body.appendChild(output);
}

function mergeLists(lists) {
    var seen = {};
    var result = [];
    
    for (var i = 0; i < lists.length; i++) {
        var items = lists[i].getElementsByTagName('li');
        for (var j = 0; j < items.length; j++) {
            var str = items[j].innerHTML;
            if (!(str in seen)) {
                result.push(str);
                seen[str] = true;
            }
        }
    }
    return result;
}
<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>
</body>

|improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy