patternjavascriptMinor
Rotating `n` random items in an array
Viewed 0 times
randomitemsrotatingarray
Problem
Honestly, it should be an incredibly simplistic function, yet it has become the following atrocity:
And as it's just for a weekend project and it works I should be happy... but honestly, I can not say I am. It feels like the function is both far too complex for the simplicity of the task and far too inefficient. Looking forward to any and all criticism and advice.
(And just for the record: I wouldn't normally have those comments in my code as I believe code should be self explanatory, but it seemed useful in this case (as in, on codereview.SE) to make the intent of the function clear.)
function rotate(arr, n){
// step 1: pick n different elements in the array
var allIndexes = [];
for(var i = 0; i<arr.length; i++){
allIndexes.push(i);
}
var pickedIndexesToRotate = [];
var valuesOfPickedIndexes = [];
for(var i = 0; i<n; i++){
var randomIndex = Math.floor(Math.random()*allIndexes.length);
valuesOfPickedIndexes.push(arr[allIndexes[randomIndex]]);
pickedIndexesToRotate.push(allIndexes.splice(randomIndex,1)[0]);
}
// step 2: actually rotate those elements...
// so if n = 3 and pickedIndexesToRotate = [5, 1, 2]
// we get [10, 60, 20, 40, 50, 30]
for(var i = 1; i<=n; i++){
arr[pickedIndexesToRotate[i == n ? 0 : i]] = valuesOfPickedIndexes.shift();
}
return arr;
}
rotate([10,20,30,40,50,60], 3);And as it's just for a weekend project and it works I should be happy... but honestly, I can not say I am. It feels like the function is both far too complex for the simplicity of the task and far too inefficient. Looking forward to any and all criticism and advice.
(And just for the record: I wouldn't normally have those comments in my code as I believe code should be self explanatory, but it seemed useful in this case (as in, on codereview.SE) to make the intent of the function clear.)
Solution
First of all, I wouldn't call it "rotate" as rotate has a significant meaning in programming. What you're doing though is picking up
Anyways, I'll just explain in the code for the rest.
Looping backwards is personal preference, because I don't have to use comparison operators, and I can always use
The
n random values and rotating them by themselves, and not in context to the array itself.Anyways, I'll just explain in the code for the rest.
function rotate(arr, n){
// I wouldn't want to mutate the original array in the operation
// so instead I operate on a shallow copy of the array.
var arrToOperate = arr.slice();
// Then we select indices by random.
// The | 0 at the end truncates the decimal part, like floor.
// We sort, since rotation needs to know who is last/first.
var selectedIndices = [];
for(var i = n; i--;) selectedIndices.push((Math.random()*arr.length) | 0);
selectedIndices.sort();
// Now the actual rotation. If value isn't the zeroth number, get the next
// otherwise get the one from the other end.
for(var j = n; j--;) arrToOperate[selectedIndices[j]] = arr[selectedIndices[(!j ? n : j) - 1]];
return arrToOperate;
}
alert(JSON.stringify(rotate([10,20,30,40,50,60], 3)));
Looping backwards is personal preference, because I don't have to use comparison operators, and I can always use
0 as the terminator value.The
| 0 is also a personal preference, as it is shorter (and sometimes faster) than Math.floor.Context
StackExchange Code Review Q#98818, answer score: 2
Revisions (0)
No revisions yet.