2
\$\begingroup\$

I have two implementations of the same function. Which of these is easier to understand?

Code sample A

function makeRectanglePoint(box, index){

    var boxToUse;

    function randomPoint(a, b){
        return Math.random() * Math.abs(a - b) + Math.min(b, a)
    }

    if(index % 2){ boxToUse = box.p1 } else { boxToUse = box.p2 }

    if(index < 2){
        return new Point(randomPoint(box.p1.x, box.p2.x), boxToUse.y);
    } else {
        return new Point(boxToUse.x, randomPoint(box.p1.y, box.p2.y));
    }

}

Code sample B

function makeRectanglePoint(rectangle, pointIndex){

    function getRectangleEdge(){

        function getPointByIndex(index) {
            switch (index) {
                case 0: return rectangle.p1;
                case 1: return new Point(rectangle.p2.x, rectangle.p1.y);
                case 2: return rectangle.p2;
                case 3: return new Point(rectangle.p1.x, rectangle.p2.y);
            }
        }

        function Line(p1, p2){
            this.p1 = p1;
            this.p2 = p2;
        }

        var endPointIndex = (pointIndex + 1) % 4;
        return new Line(
            getPointByIndex(pointIndex),
            getPointByIndex(endPointIndex)
        );
    }

    function generateRandomPointOnEdge(line){

        function generateRandomBetween(a, b){
            return Math.min(a, b) + Math.random() * Math.abs(a - b);
        }

        return new Point(
            generateRandomBetween(line.p1.x, line.p2.x),
            generateRandomBetween(line.p1.y, line.p2.y)
        );

    }

    return generateRandomPointOnEdge(getRectangleEdge());
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I'd say version A wins simply by being more succinct. Version B does however provide some extra detail, such as the fact that the point being added is being added on an edge (not inside a rectangle). Still, the structure of version A is a lot more straightforward to read.

Both version could use comments though!

I'd also replace the 1-line if..else with a simple ternary:

boxToUse = index % 2 ? box.p1 : box.p2;

Speaking of, shouldn't that be pointToUse instead of boxToUse? In either case, though, I'd ask "to use for what exactly?"

I don't know what this is used for, but I imagine a function that returns a random number in a given range would be useful elsewhere, so you might consider extracting it. Regardless, its name shouldn't be randomPoint, since it doesn't return a point - it just returns a number.

As for version B (just to give it a once over):

  • Using a constructor for Line is overkill. Just use an object literal: {p1: p1, p2: p2}

  • The switch is tricky. It has no default case and instead relies on its input being 0-3


All in all it's hard to say much more, since I don't know the context.

But a 3rd way might be better still. I'd recommend something like

// Returns a point lying on one of the rectangle's edges
// The edge is given as a number:
// 0: top edge
// 1: right edge
// 2: bottom edge
// 3: left edge
function makeRectanglePoint(rect, edge) {
  // generate a random number within the rect's height or length
  var rand = edge % 2 ? random(rect.p1.y, rect.p2.y) : random(rect.p1.x, rect.p2.x);

  switch(edge % 4) {
    case 0: return new Point(rand, rect.p1.y); // top
    case 2: return new Point(rand, rect.p2.y); // bottom
    case 1: return new Point(rect.p2.x, rand); // right
    case 3: return new Point(rect.p1.x, rand); // left
  }

  // handle errors (i.e. edge not being an integer or being negative)
  throw new Error("Edge index should be an integer between 0 and 3 (inclusive)";
}
\$\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.