0
\$\begingroup\$

I have written a function that fills in the values between points and creates one continuous path (essentially walks between the points) and I'd like some feedback on it's implementation.

If two points are provided that are not "in line" with one another then an error is thrown, as I do not want to support diagonal movement.

Here is an example of the code's input and output:

The route to walk

|_Point_|_X_|_Y_|
| A     | 0 | 0 |
| B     | 2 | 0 |
| C     | 2 | 2 |
| D     | 2 | 4 |
| E     | 4 | 4 |

Diagram of the route in 2D space

  0_1_2_3_4_X
0|A   B
1|
2|    C
3|
4|    D   E
Y|

Calling the walk function

const output = walk(
  { x: 0, y: 0 }, // A
  { x: 2, y: 0 }, // B
  { x: 2, y: 2 }, // C
  { x: 2, y: 4 }, // D
  { x: 4, y: 4 }  // E
);

The output from the walk function

[
  { x: 0, y: 0 }, // A
  { x: 1, y: 0 }, // A -> B
  { x: 2, y: 0 }, // B
  { x: 2, y: 1 }, // B -> C
  { x: 2, y: 2 }, // C
  { x: 2, y: 3 }, // C -> D
  { x: 2, y: 4 }, // D
  { x: 3, y: 4 }, // D -> E
  { x: 4, y: 4 }  // E
]

And the function itself:

function walk(...points) {
  const result = [];
  let lastPoint = null;

  points.forEach((point, index) => {
    if (lastPoint === null) {
      result.push(point);
    } else {
      const direction = {
        x: Math.sign(point.x - lastPoint.x),
        y: Math.sign(point.y - lastPoint.y)
      };

      if (direction.x !== 0 && direction.y !== 0) {
        throw new Error(`points are misaligned at ${index}`);
      } else if (direction.x % 1 !== 0 || direction.y % 1 !== 0) {
        throw new Error(`points must be whole values at ${index}`);
      } else if (direction.x === 0 && direction.y === 0) {
        return;
      }

      const axis = direction.x !== 0 ? 'x' : 'y',
            otherAxis = axis === 'x' ? 'y' : 'x';

      const start = lastPoint[axis] + direction[axis],
            comparator = direction[axis] > 0 ? (a, b) => a <= b : (a, b) => a >= b,
            delta = direction[axis];

      for (let value = start; comparator(value, point[axis]); value += delta) {
        result.push({
          [axis]: value,
          [otherAxis]: point[otherAxis]
        });
      }
    }

    lastPoint = point;
  });

  return result;
}

Things I think could be improved

  1. I don't like how the comparator variable works — it seems a bit of a hack and there's most likely a more mathematical way of working it out.

  2. Are there more "ES6-y" ways of doing what I've done? For example using reduce or similar functions.

\$\endgroup\$
2
\$\begingroup\$

This condition

else if (direction.x % 1 !== 0 || direction.y % 1 !== 0)

Doesn't check for integer and is (almost) always false, since direction.x is the result of Math.sign(), which returns an integer (1, 0, or -1) or NaN, which is the only time the condition can be true.

Use point.x instead. I would also use Number.isInteger() to check for integers. This check should also be at the start of your loop, since the first point of the input is currently not getting checked.


I'm not sure what this code is supposed to accomplish.

else if (direction.x === 0 && direction.y === 0) {
  return;
}

What is the desires behavior when you get two identical point after each other? Is it illegal? Throw an error. Should it keep both points? Don't do anything. Should duplicates be deleted? Replace return with continue so it won't get added to the result.

\$\endgroup\$
  • \$\begingroup\$ I am returning when there is no direction as it means that there is a duplicate input point which is ignored (only the first one is used). Returning actually only skips the current iteration, as I'm using Array.prototype.forEach(). This is also why I'm using return instead of continue. \$\endgroup\$ – Jack Wilsdon Sep 28 '16 at 12:52
  • 1
    \$\begingroup\$ You are right, I forgot forEach was using a callback function \$\endgroup\$ – Kruga Sep 28 '16 at 13:03

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.