# Readability of two rectangular point function samples

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());
}


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)";
}