3
\$\begingroup\$

I'm very new to the JavaScript, and hence I would like to get a feel on the language by writing some of the basic programs. One of those is the following visualization of the Bubble Sort:

Code:

<!DOCTYPE html>
<html>

  <body style="background: pink">

    <p id="word" style="color: black">Bubble sort: visualization</p>
    <button onclick=CreateBarPlot()>Generate data</button>
    <button onclick=bubble_sort()>Sort</button>



    <script>
      var BARS_DATA = []

      function GenerateNumbers(count = 15) {
        var numbers = [];
        var i;
        for (i = 0; i < count; i++) {
          numbers.push(Math.floor(Math.random() * (70 - 1)) + 1);
        }
        return numbers
      }


      function CreateBar(index, value) {

        const div = document.createElement("div");

        //Set ID of the div
        div.id = 'bar' + index

        //Configure text properites
        div.innerHTML = value;
        div.style.color = 'black';
        div.style.textAlign = "right";

        //Configure bar properties
        div.style.background = 'lightblue';
        div.style.padding = '2.5px';
        div.style.margin = '1px';
        div.style.width = (value * 10) + 'px'

        return div
      }


      function CreateBarPlot() {
        //Refresh the BAR_DATA
        BARS_DATA = []

        //Refresh the barplot if one has been already generated
        if (document.getElementById("bar_plot") != null) {
          document.getElementById("bar_plot").remove()
        }

        var bar_plot = document.createElement("p");
        bar_plot.id = "bar_plot"


        numbers = GenerateNumbers()
        var i;
        for (i = 0; i < numbers.length; i++) {
          var cur_bar = CreateBar(i, numbers[i]);
          bar_plot.appendChild(cur_bar);
          BARS_DATA.push(cur_bar)
        }
        document.body.append(bar_plot)
      }


      function bubble_sort(i = 1, switched = false) {

        if (i > 1) {
          BARS_DATA[i - 1].style.background = 'lightblue';
          BARS_DATA[i - 2].style.background = 'lightblue';
        }

        if (i == BARS_DATA.length && switched == false) {
          return true;
        } else if (i == BARS_DATA.length && switched == true) {
          i = 1;
          switched = false;
        }


        BARS_DATA[i - 1].style.background = 'red';
        BARS_DATA[i].style.background = 'red';
        setTimeout(function() {


          //Highlight the bars that are about to be compared
          var left_val = parseInt(BARS_DATA[i - 1].innerHTML);
          var right_val = parseInt(BARS_DATA[i].innerHTML);


          if (left_val > right_val) {
            switched = true;
            switch_bars(BARS_DATA[i - 1], BARS_DATA[i])
          }

          bubble_sort(i + 1, switched)
        }, 50);

      }


      function switch_bars(bar1, bar2) {

        bar1_value = bar1.innerHTML
        bar1_width = bar1.style.width

        bar2_value = bar2.innerHTML
        bar2_width = bar2.style.width


        //Perform switch
        bar1.innerHTML = bar2_value
        bar1.style.width = bar2_width

        bar2.innerHTML = bar1_value
        bar2.style.width = bar1_width
      }

    </script>


  </body>

</html>

To run it, see the JSFiddle

What can be improved?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Regarding

<body style="background: pink">
  <p id="word" style="color: black">Bubble sort: visualization</p>
  <button onclick=CreateBarPlot()>Generate data</button>
  <button onclick=bubble_sort()>Sort</button>

It's not a good idea to use inline event handlers in modern Javascript, because they require global pollution and can have some very strange behavior. Leave the HTML markup to be the actual content of the site - keep the styling and Javascript to their own self-contained sections instead, preferably. To attach event listeners to an element, use addEventListener.

In the Javascript, you have some places where you assign to different .style properties of an element. Consider adding CSS rules instead. Here, the bars are children of the #bar_plot, so all you need is the #bar_plot > div selector - you don't even need to add a class to the elements. Of course, when the property you want is dynamically calculated in the JS, like div.style.width = (value * 10) + 'px', you need to use JS, but otherwise, better to use CSS rules.

You use div.innerHTML = value;. Unless you're deliberately inserting HTML markup, it's faster, safer, and more predictable if you use the textContent property instead, if you want to put text inside an element. (Even if you're already sure value is plain text, and doesn't contain HTML markup, other readers of the code may not be as sure as you on casual inspection, which may cause worries and double-checks) Same thing for when retrieving element content - unless you need to retrieve HTML markup, use textContent.

You declare a bar element with:

const div = document.createElement("div");

const is ES2015 syntax. ES2015 syntax is great - it often makes code more readable and concise. I'd highly recommend using it everywhere. If you need compatibility with completely obsolete browsers (like IE), the professional thing to do is to use Babel to transpile your clean ES2015+ code into ES5 syntax.

But whatever you decide, make sure to have a consistent style. If you really want to write in ES5 for some reason, then best to do so everywhere - otherwise, use ES2015+. (Don't mix var and const / let).

Either way, also make sure to declare your variables before using them - your numbers variable (and bar1_value, and bar1_width, and more) are not declared, which means that when you assign to it, you're implicitly creating a global variable. Or, if you use strict mode (which you should, it turns potential bugs like these into early errors), an error will be thrown.

Elements with IDs automatically become global variables with the name of the ID. This is weird and can cause bugs and confusion. I'd prefer to avoid IDs when possible, or at least minimize their usage to only absolutely unique elements. Dynamic numeric-indexed IDs definitely should be avoided (especially since they aren't being referenced anywhere else anyway).

The overwhelming majority of professional Javascript uses camelCase for nearly all variable names (including functions). PascalCase is generally reserved for classes and constructor functions, which you aren't using here. You can consider whether you want to conform to the de-facto standard.

You're sometimes putting semicolons at the end of statements, and sometimes you aren't. Omitting semicolons can occasionally result in bugs, especially if you're a beginner - even if you know the rules of ASI, it's best to be consistent with a code style. Consider using a linter like ESLint to automatically prompt you to fix these sorts of inconsistencies and potential bugs.

BARS_DATA is a confusing name. It's declared as an array, and it sounds like it holds data, which I would intuitively think would be the numeric data points, but it actually holds elements. Maybe call it barElements instead. Or, even better, you can avoid the global array there entirely by selecting the rows from the DOM when needed instead.

With bubble sort, after a full initial iteration through the array, the value at the last index will be completely sorted (the final value there). After the second full iteration, the second-last value will be completely sorted, and so on. You can improve your logic by iterating one less element each time, and perhaps make the logic clearer by giving these completely-sorted elements a style change.

Rather than bubbleSort recursively calling itself with index and changed arguments, I think the logic might be clearer if it only ran once instead, and added delays by awaiting a Promise that resolves after a couple seconds.

It's possible to click the sort button more than once, resulting in multiple sorts occurring simultaneously, which doesn't make sense. Perhaps disable the buttons while sorting.

Put it all together, and you get something like:

'use strict';

// you could also put the whole script into an IIFE to avoid globals

function generateNumbers(count = 15) {
  const numbers = [];
  for (let i = 0; i < count; i++) {
    numbers.push(Math.floor(Math.random() * (70 - 1)) + 1);
  }
  return numbers;
}

function createBar(index, value) {
  const div = document.createElement("div");
  div.textContent = value;
  div.style.width = (value * 10) + 'px';
  return div;
}

function createBarPlot() {
  const existingPlot = document.querySelector('.bar-plot');
  if (existingPlot) {
    existingPlot.remove();
  }
  const barPlot = document.body.appendChild(document.createElement("p"));
  barPlot.className = 'bar-plot';
  const numbers = generateNumbers();
  for (let i = 0; i < numbers.length; i++) {
    barPlot.appendChild(createBar(i, numbers[i]));
  }
}

const delay = ms => new Promise(resolve => setTimeout(resolve, ms));
async function bubbleSort() {
  const barElements = [...document.querySelectorAll('.bar-plot > div')];

  // Some helper functions first:
  const getNum = i => Number(barElements[i].textContent);
  const switchBars = (i) => {
    barElements[i].insertAdjacentElement('afterend', barElements[i - 1]);
    // Swap the positions of the bars in the array of elements:
    [barElements[i], barElements[i - 1]] = [barElements[i - 1], barElements[i]];
  };

  for (let cyclesToGo = barElements.length - 1; cyclesToGo >= 0; cyclesToGo--) {
    // Add active class to first element:
    barElements[0].classList.add('active');
    for (let i = 1; i <= cyclesToGo; i++) {
      // Add active class to next element:
      barElements[i].classList.add('active');
      if (getNum(i - 1) > getNum(i)) {
        switchBars(i);
      }
      await delay(50);
      // Remove active class from last element:
      barElements[i - 1].classList.remove('active');
    }
    // Cycle complete; last one iterated over is in its final position
    barElements[cyclesToGo].className = 'done';
  }
  generateBtn.disabled = false;
}

const buttons = document.querySelectorAll('button');
const [generateBtn, sortBtn] = buttons;
const setDisabled = newDisabled => {
  for (const button of buttons) {
    button.disabled = newDisabled;
  }
};
generateBtn.addEventListener('click', () => {
  setDisabled(false);
  createBarPlot();
});
sortBtn.addEventListener('click', () => {
  setDisabled(true);
  bubbleSort();
});
body {
  background: pink;
}
.bar-plot > div {
  color: black;
  text-align: right;
  background: lightblue;
  padding: 2.5px;
  margin: 1px;
}
.bar-plot > div.active {
  background: red;
}
.bar-plot > div.done {
  background: yellow;
}
<p>Bubble sort: visualization</p>
<button>Generate data</button>
<button>Sort</button>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.