4
\$\begingroup\$

I have a function that gets a random number between a min and max.

I call the function on a variable as such:

let randomPage = getRandomIntInclusive(1, 5);

That expression is repeated several times in my code (let's say 5 to 10 times within different functions). Is there a way to refactor it further or call it in a better way?

The way I call it is like:

function a() {
  randomPage = getRandomIntInclusive(1, 5);
}

function b() {
  randomPage = getRandomIntInclusive(1, 5);
}

function c() {
  randomPage = getRandomIntInclusive(1, 5);
}

Full code:

$http({
  method: "GET",
  url: "URL"
})
.then(function mySuccess(response) {
  $scope.discoverMovie = response.data;
  /* console.log($scope.discoverMovie);
  console.log($scope.discoverMovie.results[0].id); */
  var i = getRandomIntInclusive(1, $scope.discoverMovie.results.length - 1);
  trailerKey = $scope.discoverMovie.results[i].id;
  getMovieDetail(trailerKey);

  $scope.nextMovie = function () {
    /* Get Movie to Next Movie Function */
    i = i + 1;
    if (i == $scope.discoverMovie.results.length) {
      randomPage = getRandomIntInclusive(1, 5);
      i = getRandomIntInclusive(1, $scope.discoverMovie.results.length - 1);
      getMovieList(randomPage);
    }
    trailerKey = $scope.discoverMovie.results[i].id;
    getMovieDetail(trailerKey);
  };

  $scope.previousMovie = function () {
    /* Get Movie to Next Movie Function */
    i = i - 1;
    if (i < 0) {
      randomPage = getRandomIntInclusive(1, 5);
      i = getRandomIntInclusive(0, $scope.discoverMovie.results.length - 1);
      getMovieList(randomPage);
    }
    trailerKey = $scope.discoverMovie.results[i].id;
    getMovieDetail(trailerKey);
  };
}, function myError(response) {
    $scope.discoverMovie = response.statusText;
 });
\$\endgroup\$
  • \$\begingroup\$ You don't provide enough context in this question. Show your real code, because based on it the solutions may be very different (Guarang's answer may or may not be good). \$\endgroup\$ – Igor Soloydenko Dec 27 '17 at 21:17
  • \$\begingroup\$ @IgorSoloydenko i have added the rest of the code. basically i am wondering if i can avoid repeating randomPage = getRandomIntInclusive(1, 5); \$\endgroup\$ – Krishna Dec 27 '17 at 22:26
  • \$\begingroup\$ This code makes more sense now. And I feel I like the answer you have already got. In a different context a higher-order function might get handy which is probably not the case here. \$\endgroup\$ – Igor Soloydenko Dec 27 '17 at 22:31
3
\$\begingroup\$

You can abstract the code in another function and then call it.

If you are using OOP, then you can either pull that in (abstract) Parent class, this way it would be accessible to everyone.

function getRandomPageNumber(){
    return getRandomIntInclusive(1, 5);
}
\$\endgroup\$
  • \$\begingroup\$ #Gaurang right loool that easy. sooo would you write it then as randomPage= getRandomPageNumber();? \$\endgroup\$ – Krishna Dec 27 '17 at 22:15
  • \$\begingroup\$ @Krishna yes, but the key here is that you abstract out the generation of random number. You don't need to bear those magic numbers 1 and 5 wherever you need a random. I'd recommend making it a function of $scope though, then you have access to discoverMovie.results... \$\endgroup\$ – Igor Soloydenko Dec 27 '17 at 22:33
  • \$\begingroup\$ @IgorSoloydenko ... Thanks for pointing it out. Ben Rudgers also pointed that out. So I will dfntly make the change. Thanks again. \$\endgroup\$ – Krishna Dec 27 '17 at 23:16
3
\$\begingroup\$

If the same values (1 and 5 in your example code) are being called repeatedly, they should be given meaningful names. For example:

var firstAvailablePage = 1;
var lastAvailablePage = 5;

or perhaps better:

const firstAvailablePage = 1;
const lastAvailablePage = 5

otherwise, 1 and 5 are magic numbers. Magic numbers make the code hard to understand and brittle when refactoring because each occurrence of each magic constant has to be manually changed.

Even with just a single use, naming the values makes the code easier to read and helps the reader understand why 1 is one (and not some other value) and 5 is five.

\$\endgroup\$
  • 1
    \$\begingroup\$ you right... did not cross my mind. but giving them meaningfull names is dfntly easier to read. thanks for that. \$\endgroup\$ – Krishna Dec 27 '17 at 23:15

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.