CODEWITHSUNDEEP
javascriptrefactoring
Wonkook Lee
Wonkook Lee

Reputation: 37

refactoring: How can I improve this as a clean code

I am currently studying JS myself.
And make the pig-game which is project of the course that I watched.
I'm always curious how can I improve my code but I got no idea where do I begin.
Is there some principle that I can improve any code?
If there's a way, Could you let me know?
Thanks!
https://github.com/wonkooklee/pig-game
result : https://wonkooklee.github.io/pig-game/

below is main functions

document.querySelector('.btn-roll').addEventListener('click', function() {
  
  if (gamePlaying) {
    
    dice = Math.floor(Math.random() * 6) + 1;
    diceDOM.style.display = 'block';
    diceDOM.src = `dice-${dice}.png`;

    if (dice === 6 && lastDice === 6) {
      // Player looses score
      scores[activePlayer] = 0;
      document.getElementById(`score-${activePlayer}`).textContent = '0';
      nextPlayer();
    } else if (dice !== 1 && dice !== 6) {
      roundScore += dice;
      document.getElementById(`current-${activePlayer}`).textContent = roundScore;
      lastDice = 0;
    } else if (dice === 6) {
      roundScore += dice;
      document.getElementById(`current-${activePlayer}`).textContent = roundScore;
      lastDice = dice;
    } else {
      nextPlayer();
    }


  }

});

document.querySelector('.btn-hold').addEventListener('click', function() {
  if (gamePlaying) {
    scores[activePlayer] += roundScore;
    document.getElementById(`score-${activePlayer}`).textContent = scores[activePlayer];

    let input = document.getElementById('scoreSet').value;
    let winningScore;

    if (isNaN(input) === false) {
      winningScore = input;
    } else {
      document.getElementById('scoreSet').value = '100';
    }

    if (scores[activePlayer] >= winningScore) {

      document.getElementById(`name-${activePlayer}`).textContent = 'WINNER!';
      document.querySelector(`.player-${activePlayer}-panel`).classList.add('winner');
      document.querySelector(`.player-${activePlayer}-panel`).classList.remove('active');
      diceDOM.style.display = 'none';
      gamePlaying = false;
    } else {
      nextPlayer();
    }

  }

});

my pig-game

Upvotes: 0

Views: 75

Answers (1)

Anton Tokmakov
Anton Tokmakov

Reputation: 710

Martin Fowler wrote a great book "Refactoring". Besides, Fowler has a great blog Refactoring.com, where you can find a lot of information about refactoring with examples in Javascript. I'm not strong in Javascript, but can let you some advices about your code.

1.Simplify Conditional Logic

For example like this:

if (dice === 6 && lastDice === 6) {
  // Player looses score
  scores[activePlayer] = 0;
  document.getElementById(`score-${activePlayer}`).textContent = '0';
  nextPlayer();
  return;
} 

if (dice !== 1 && dice !== 6) {
  roundScore += dice;
  document.getElementById(`current-${activePlayer}`).textContent = roundScore;
  lastDice = 0;
  return;
} 

if (dice === 6) {
  roundScore += dice;
  document.getElementById(`current-${activePlayer}`).textContent = roundScore;
  lastDice = dice;
  return;
}

nextPlayer();

2.Delete duplicated code and extract function

For example

function someFunctionName(diffRoundScore, lastDiceValue){
  roundScore += diffRoundScore;
  document.getElementById(`current-${activePlayer}`).textContent = roundScore;
  lastDice = lastDiceValue;
}

if (dice !== 1 && dice !== 6) {
  someFunctionName(dice, 0);
  return;
} 

if (dice === 6) {
  someFunctionName(dice, dice);
  return;
}

3.Change check "dice for 6" to function

function isDiceEqualsSix { return dice === 6};

if (isDiceEqualsSix && lastDice === 6) {
  // Player looses score
  scores[activePlayer] = 0;
  document.getElementById(`score-${activePlayer}`).textContent = '0';
  nextPlayer();
  return;
} 

if (dice !== 1 && !isDiceEqualsSix) {
  someFunctionName(dice, 0);
  return;
} 

if (isDiceEqualsSix) {
  someFunctionName(dice, dice);
  return;
}

4.Change "6" to a constant variable or a function

const LIMIT_SCORE = 6;

function isDiceEqualsSix { return dice === LIMIT_SCORE};

if (isDiceEqualsSix && lastDice === LIMIT_SCORE) {
  // Player looses score
  scores[activePlayer] = 0;
  document.getElementById(`score-${activePlayer}`).textContent = '0';
  nextPlayer();
  return;
}

I hope I helped you.

Upvotes: 1

Related Questions