4
\$\begingroup\$

Honestly, it should be an incredibly simplistic function, yet it has become the following atrocity:

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.)

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

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 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.

\$\endgroup\$

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.