2
\$\begingroup\$

I have a JavaScript function (which uses the jQuery library but that should not be relevant here) that takes as input an ID (integer), a color, and a side (all, top, right, bottom, or left). For each side, a computation is run in an if-clause. For instance, of borderSide == "top" and anId >= width, do something.

Even though it's not important for the problem, it might help to understand what is going on: the idea is that in a table with a width and height of cells, a user can "color" a cell's border. However, in HTML/CSS it might be necessary to also color another cell's border to make the effect visible. E.g. if you want to color a cell's top border, you may need to paint the cell right above it in the same color as well.

"all" is a special case, as that means all sides of a cell need to be painted, and with respect to its neighbours all surrounding cells need to undergo modification as well (top's bottom border, right's left, bottom's top, left's right). The disjunction between the case with all and all others makes me use a lot of duplicate code, however I feel that the only way I could minimise this is by using separate if's (instead of if...else if's). However, that would mean that every if is evaluated and needs computation, even though in the cases without all there can only be one if active at any given time. So my question is, how can I re-write the following and getting rid off duplication, without evaluating each if-clause if borderSide is not all?

function paintConnectedBorder(cellId, selectedBorderColor, selectedBorderSide) {
  const width = $("#pixels").data("defaultGrid").grid.width,
    totalCells = width * $("#pixels").data("defaultGrid").grid.height;

  let changedCells = [];

  if (selectedBorderSide === "all") {
    if (cellId >= width) {
      $("#cell-" + (cellId - width)).css("borderBottomColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - width);
    }
    if (cellId % width - 1 !== 0) {
      $("#cell-" + (cellId + 1)).css("borderLeftColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + 1);
    }
    if (cellId < totalCells - width - 1) {
      $("#cell-" + (cellId + width)).css("borderTopColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + width);
    }
    if (cellId % width !== 0) {
      $("#cell-" + (cellId - 1)).css("borderRightColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - 1);
    }
  } else {
    if (selectedBorderSide === "top" && cellId >= width) {
      $("#cell-" + (cellId - width)).css("borderBottomColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - width);
    } else if (selectedBorderSide === "right" && (cellId % width - 1 !== 0)) {
      $("#cell-" + (cellId + 1)).css("borderLeftColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + 1);
    } else if (selectedBorderSide === "bottom" && cellId < totalCells - width - 1) {
      $("#cell-" + (cellId + width)).css("borderTopColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + width);
    } else if (selectedBorderSide === "left" && (cellId % width !== 0)) {
      $("#cell-" + (cellId - 1)).css("borderRightColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - 1);
    }
  }
  return changedCells;
}

So if borderSide === "all", all conditions after the &&'s should be evaluated. However, if it's not all, there can only be one at a time (hence the use of else if). So when it's not "all", I do not want to evaluate all if-clauses. So when borderSide === "top" there is no chance that any of the following if-clauses will be true, so I don't want to evaluate them and thus save computations.

\$\endgroup\$
1
\$\begingroup\$

Recursion is a handy way to remove repetition...

function paintConnectedBorder(cellId, selectedBorderColor, selectedBorderSide) {
  const width = $("#pixels").data("defaultGrid").grid.width,
    totalCells = width * $("#pixels").data("defaultGrid").grid.height;

  let changedCells = [];

  if (selectedBorderSide === "all") {
    ['top','bottom','left','right'].forEach(side=>
        changedCells.push(...paintConnectedBorder(cellId, selectedBorderColor, side)));
  } else {
    if (selectedBorderSide === "top" && cellId >= width) {
      $("#cell-" + (cellId - width)).css("borderBottomColor", selectedBorderColor);
      changedCells.push("cell-" + anId - width);
    } else if (selectedBorderSide === "right" && (cellId % width - 1 !== 0)) {
      $("#cell-" + (cellId + 1)).css("borderLeftColor", selectedBorderColor);
      changedCells.push("cell-" + anId + 1);
    } else if (selectedBorderSide === "bottom" && anId < totalCells - width - 1) {
      $("#cell-" + (cellId + width)).css("borderTopColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + width);
    } else if (selectedBorderSide === "left" && (cellId % width !== 0)) {
      $("#cell-" + (cellId - 1)).css("borderRightColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - 1);
    }
  }
  return changedCells;
}

Basically if 'all' is selected the function will call itself once for each of the individual selections.


As requested, here's the ES5 version for older browsers...

function paintConnectedBorder(cellId, selectedBorderColor, selectedBorderSide) {
  const width = $("#pixels").data("defaultGrid").grid.width,
    totalCells = width * $("#pixels").data("defaultGrid").grid.height;

  let changedCells = [];

  if (selectedBorderSide === "all") {
    var sides = ['top','bottom','left','right'];
    for(var i=0, side; side=sides[i]; i++)
        changedCells.push.apply(changedCells, paintConnectedBorder(cellId, selectedBorderColor, side)));
  } else {
    if (selectedBorderSide === "top" && cellId >= width) {
      $("#cell-" + (cellId - width)).css("borderBottomColor", selectedBorderColor);
      changedCells.push("cell-" + anId - width);
    } else if (selectedBorderSide === "right" && (cellId % width - 1 !== 0)) {
      $("#cell-" + (cellId + 1)).css("borderLeftColor", selectedBorderColor);
      changedCells.push("cell-" + anId + 1);
    } else if (selectedBorderSide === "bottom" && anId < totalCells - width - 1) {
      $("#cell-" + (cellId + width)).css("borderTopColor", selectedBorderColor);
      changedCells.push("cell-" + cellId + width);
    } else if (selectedBorderSide === "left" && (cellId % width !== 0)) {
      $("#cell-" + (cellId - 1)).css("borderRightColor", selectedBorderColor);
      changedCells.push("cell-" + cellId - 1);
    }
  }
  return changedCells;
}
\$\endgroup\$
  • \$\begingroup\$ Ah yes, this is what I was aiming for! The only question that I have, why the ... inside the push inside forEach? \$\endgroup\$ – Bram Vanroy Dec 20 '17 at 19:54
  • 1
    \$\begingroup\$ @BramVanroy - The spread operator is used to separate all the values in the array into separate arguments for the push method to consume. Without the spread you'll be creating an array of arrays rather than a flat array. \$\endgroup\$ – I wrestled a bear once. Dec 20 '17 at 20:04
  • \$\begingroup\$ @BramVanroy - here's a quick demo to show how it works. \$\endgroup\$ – I wrestled a bear once. Dec 20 '17 at 20:09
  • \$\begingroup\$ I had never heard of the spread operator! That's so cool! For backwards-compatibility's sake, could you add a snippet for older browsers that do not support it? I'm assuming push.apply(null, someFunction(blabla)? \$\endgroup\$ – Bram Vanroy Dec 21 '17 at 6:42
  • \$\begingroup\$ yessir I'll add an es5 version when I get a second today, but yea, apply would be the workaround. I'd probably use the array as the context instead of null tho \$\endgroup\$ – I wrestled a bear once. Dec 21 '17 at 12:09

Your Answer

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

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