3
\$\begingroup\$

I'm writing a simple tool as a helper on Route Cipher for fun. In summary, the tool breaks input string into a two-dimensional array of characters with the dimensions specified by the user, and after that it's represented as a table on HTML. The tool also has a function to shift table data on a row to left or right, and shift table data on a column to up or down, represented with a button next to each row/column.

As this is my first project implementing Javascript, maybe my code's too complicated or somewhat confusing. I'm looking for advice to improve it, particularly on makeGrid() because I think it's too redundant.

var rows;
var cols;
var workArr;

document.getElementById("btn-makeGrid").addEventListener("click", parseInput);

function parseInput() {
  if (document.getElementById("grid") !== null) {
    deleteGrid();
  }

  var input = document.getElementById("input").value;
  rows = parseInt(document.getElementById("gridRow").value);
  cols = parseInt(document.getElementById("gridCol").value);

  workArr = [];
  for (var i = 0; i < rows; i++) {
    workArr[i] = [];
    for (var j = 0; j < cols; j++) {
      workArr[i][j] = input[i * cols + j];
    }
  }

  makeGrid(workArr);
}

function makeGrid(workArr) {
  var output = document.getElementById("output");
  var table = document.createElement("table");
  table.id = "grid";

  //Line 32-48: Up Shift Button
  var tr = document.createElement("tr");
  table.appendChild(tr);

  var td = document.createElement("td");
  tr.appendChild(td);
  for (var i = 0; i < cols; i++) {
    var td = document.createElement("td");
    var button = document.createElement("button");
    button.className = "shift";
    button.id = "u-" + i;
    button.innerHTML = '<i class="fa fa-angle-up"></i>';
    td.appendChild(button);
    tr.appendChild(td);
  }
  var td = document.createElement("td");
  tr.appendChild(td);

  for (var i = 0; i < rows; i++) {
    var tr = document.createElement("tr");
    table.appendChild(tr);

    //Line 54-61: Left Shift Button
    var td = document.createElement("td");
    var button = document.createElement("button");
    button.className = "shift";
    button.id = "l-" + i;
    button.innerHTML = '<i class="fa fa-angle-left"></i>';
    td.appendChild(button);
    tr.appendChild(td);

    //Line 63-69: Contents of workArr
    for (var j = 0; j < cols; j++) {
      var td = document.createElement("td");
      var content = document.createTextNode(workArr[i][j]);
      td.appendChild(content);
      tr.appendChild(td);
    }

    //Line 71-78: Right Shift Button
    var td = document.createElement("td");
    var button = document.createElement("button");
    button.className = "shift";
    button.id = "r-" + i;
    button.innerHTML = '<i class="fa fa-angle-right"></i>';
    td.appendChild(button);
    tr.appendChild(td);
  }

  //Line 81-97 : Down Shift Button
  var tr = document.createElement("tr");
  table.appendChild(tr);

  var td = document.createElement("td");
  tr.appendChild(td);
  for (var i = 0; i < cols; i++) {
    var td = document.createElement("td");
    var button = document.createElement("button");
    button.className = "shift";
    button.id = "d-" + i;
    button.innerHTML = '<i class="fa fa-angle-down"></i>';
    td.appendChild(button);
    tr.appendChild(td);
  }
  var td = document.createElement("td");
  tr.appendChild(td);

  output.appendChild(table);

  //Line 101-105: Add Event Listener to each Shift Button
  var shiftButtons = document.getElementsByClassName("shift");
  for (var i = 0; i < shiftButtons.length; i++) {
    shiftButtons[i].addEventListener("click", shiftFunction);
  }
}

function deleteGrid() {
  var grid = document.getElementById("grid");
  grid.remove();
}

function shiftFunction() {
  //Shift Button ID Format
  //Index 0 : Shift direction
  //Index 1 : Dash character (-)
  //Index 2 until end : Index of row/column
  var direction = this.id[0];
  var actIndex = parseInt(this.id.substring(2));
  if (direction === "l") {
    shiftLeft(workArr, actIndex);
  } else if (direction === "r") {
    shiftRight(workArr, actIndex);
  } else if (direction === "u") {
    shiftUp(workArr, actIndex);
  } else if (direction === "d") {
    shiftDown(workArr, actIndex);
  }
}

function shiftUp(workArr, colIndex) {
  for (var i = 0; i < rows - 1; i++) {
    var temp = workArr[i][colIndex];
    workArr[i][colIndex] = workArr[i + 1][colIndex];
    workArr[i + 1][colIndex] = temp;
  }

  deleteGrid();
  makeGrid(workArr);
}

function shiftDown(workArr, colIndex) {
  for (var i = rows - 1; i > 0; i--) {
    var temp = workArr[i][colIndex];
    workArr[i][colIndex] = workArr[i - 1][colIndex];
    workArr[i - 1][colIndex] = temp;
  }

  deleteGrid();
  makeGrid(workArr);
}

function shiftLeft(workArr, rowIndex) {
  for (var i = 0; i < cols - 1; i++) {
    var temp = workArr[rowIndex][i];
    workArr[rowIndex][i] = workArr[rowIndex][i + 1];
    workArr[rowIndex][i + 1] = temp;
  }

  deleteGrid();
  makeGrid(workArr);
}

function shiftRight(workArr, rowIndex) {
  for (var i = cols - 1; i > 0; i--) {
    var temp = workArr[rowIndex][i];
    workArr[rowIndex][i] = workArr[rowIndex][i - 1];
    workArr[rowIndex][i - 1] = temp;
  }

  deleteGrid();
  makeGrid(workArr);
}

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

You're constantly redeclaring the tr and td variables, this is not needed. Use var once at point of instantiation, then just simply assign to the variable afterwards.

Additionally, you've gone through the effort of adding comments for signalling that certain parts of your code carry a separate function. That's good, but we can do better: actually turn them into functions.

var td = document.createElement("td");
var button = document.createElement("button");
button.className = "shift";
button.id = "d-" + i;
button.innerHTML = '<i class="fa fa-angle-down"></i>';
td.appendChild(button);
tr.appendChild(td);

This could be a function called makeDownshiftButton(tr, i) with an argument for the tr to add the button to and i for the id. Similarily, you could have such functions for the left, right and up shift. That'd greatly simplify your code.

\$\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.