mlimberg/m2-refactor.md

## m2-refactor.md

      
    Raw
  

              m2-refactor.md
            
          
    Refactoring

Refactoring is the process of improving the structure and readability of code without changing its functionality.

Why refactor?


Prevent decay
Preserve or fix design
Reduce duplication
Improve maintainability
Helps us code faster
Locate bugs
Code smells

When to refactor?


No “special” time
Short bursts
Refactor to gain something
Prior to adding functionality
When fixing a bug
During code review
If you don't understand your own code

What Smells?????


smells are certain structures in the code that indicate violation of fundamental design principles
and negatively impact design quality". Code smells are usually not bugs—they are not technically
incorrect and do not currently prevent the program from functioning. Instead, they indicate weaknesses
in design that may be slowing down development or increasing the risk of bugs or failures in the future.
Bad code smells can be an indicator of factors that contribute to technical debt.
Common code smells

Application-level smells:

Duplicated code: identical or very similar code exists in more than one location.
Contrived complexity: forced usage of overcomplicated design patterns where simpler design would suffice.

Class-level smells:

Large class: a class that has grown too large. See God object.
Feature envy: a class that uses methods of another class excessively.
Inappropriate intimacy: a class that has dependencies on implementation details of another class.
Refused bequest: a class that overrides a method of a base class in such a way that the contract of the base class is not honored by the derived class. See Liskov substitution principle.
Lazy class / freeloader: a class that does too little.
Excessive use of literals: these should be coded as named constants, to improve readability and to avoid programming errors.    * Additionally, literals can and should be externalized into resource files/scripts where possible, to facilitate localization of software if it is intended to be deployed in different regions.
Cyclomatic complexity: too many branches or loops; this may indicate a function needs to be broken up into smaller functions, or that it has potential for simplification.
Downcasting: a type cast which breaks the abstraction model; the abstraction may have to be refactored or eliminated.[7]
Orphan variable or constant class: a class that typically has a collection of constants which belong elsewhere where those constants should be owned by one of the other member classes.
Data clump: Occurs when a group of variables are passed around together in various parts of the program. In general, this suggests that it would be more appropriate to formally group the different variables together into a single object, and pass around only this object instead.[8][9]

Method-level smells:

Too many parameters: a long list of parameters is hard to read, and makes calling and testing the function complicated. It may indicate that the purpose of the function is ill-conceived and that the code should be refactored so responsibility is assigned in a more clean-cut way.
Long method: a method, function, or procedure that has grown too large.
Excessively long identifiers: in particular, the use of naming conventions to provide disambiguation that should be implicit in the software architecture.
Excessively short identifiers: the name of a variable should reflect its function unless the function is obvious.
Excessive return of data: a function or method that returns more than what each of its callers needs.

Examples:

Duplicated Code:

Original
$('#easyGame').on('click', function() {
  let isEasy = true;

  gameStartButtonPage (isEasy);
});

$('#hardGame').on('click', function() {
  let isEasy = false;

  gameStartButtonPage (isEasy);
});

{
  background-color: #000;
  color: rgba(0, 255, 0, 1);
  font-size: 24px;
  padding: 15px 30px;
  width: 150px;
}

#easyGame:hover,
#hardGame:hover {
  background-color: rgba(0, 255, 0, 1);
  color: #000;
}

#easyGame,
#hardGame {
  height: 60px;
}

#easyGame {
  position: absolute;
  top: 250px;
  left: 480px;
}

#hardGame {
  position: absolute;
  top: 250px;
  right: 460px;
}


Refactor
$('.game-difficulty-opt').on('click', function(e) {
  const gameDifficulty = e.target.value.toLowerCase() === 'easy' ? true : false

  gameStartButtonPage(gameDifficulty);
});

canvas {
  background-color: #000;
}

body {
  text-align: center;
  box-sizing: border-box;
  position: relative;
}

#logo {
  position: absolute;
  top: 70px;
  left: 400px;
}


.game-difficulty-opt {
  background-color: #000;
  color: rgba(0, 255, 0, 1);
  font-size: 24px;
  padding: 15px 30px;
  width: 150px;
  height: 60px;
  top: 250px;
  position: absolute;
}

.game-difficulty-opt:hover {
  background-color: rgba(0, 255, 0, 1);
  color: #000;
}

#easyGame {
  left: 480px;
}

#hardGame {
  right: 460px;
}

Duplicated Code: part 2

Take a look at these functions. Do you notice similarities? How do they differ?
function generateTurtles(isEasy) {
  let x = 0;
  let pace;

  if (isEasy) {
    pace = 2;
  } else if (isEasy === false) {
    pace = 4;
  }

  for (let i = 0; i < 6; i++) {
    let turtles = new Turtle(x, 65, 80, 30, pace);

    x += 200;
    turtleArray.push(turtles);
  }
  return turtleArray;
}

function generateLogs(isEasy) {
  let x = 900;
  let pace;

  if (isEasy) {
    pace = 2;
  } else if (isEasy === false) {
    pace = 4;
  }

  for (let i = 0; i < 6; i++) {
    let logs = new Log(x, 110, 85, 30, pace);

    x += 200;
    logArray.push(logs);
  }
  return logArray;
}

function generateLeftCars(isEasy) {
  let x = 900;
  let pace;

  if (isEasy) {
    pace = 2;
  } else if (isEasy === false) {
    pace = 4;
  }

  for (let i = 0; i < 4; i++) {
    let cars = new Car(x, 245, 100, 40, pace);

    x -= 200;
    carArrayLeft.push(cars);
  }
  return carArrayLeft;
}

function generateRightTrucks(isEasy) {
  let x = 0;
  let pace;

  if (isEasy) {
    pace = 2;
  } else if (isEasy === false) {
    pace = 4;
  }

  for (let i = 0; i < 4; i++) {
    let trucks = new Truck(x, 335, 100, 40, pace);

    x += 200;
    truckArray.push(trucks);
  }
  return truckArray;
}

These functions are VERY similar. Nearly identical with a few subtle differences:

where x starts
number of obstacles
type of obstacle

Perhaps we can remove some of the duplicated code by creating another function that can be used by each obstacle type. Additionally, because each function sets pace the same way, we should make that a global variable that each obstacle function can use.
First let's set a global variable for pace by tweaking the already-refactored code from before:
// in global variables

let pace;

$('.game-difficulty-opt').on('click', function(e) {
  pace = e.target.value.toLowerCase() === 'easy' ? 2 : 4

});

And because we can now access pace globally, we no longer need to pass it to every function that generates obstacles:
Original
$('.game-difficulty-opt').on('click', function(e) {
  let gameDifficulty = e.target.value.toLowerCase() === 'easy' ? true : false

  gameStartButtonPage(gameDifficulty);
});

function gameStartButtonPage (isEasy) {
  $('#game').show();
  $('#startButton').hide();
  $('#logo').hide();
  $('#easyGame').hide();
  $('#hardGame').hide();
  generateTurtles(isEasy);
  generateLogs(isEasy);
  generateLeftCars(isEasy);
  generateRightTrucks(isEasy);
}

Refactor
$('.game-difficulty-opt').on('click', function(e) {
  pace = e.target.value.toLowerCase() === 'easy' ? true : false

  gameStartButtonPage();
});

function gameStartButtonPage () {
  $('#game').show();
  $('#startButton').hide();
  $('#logo').hide();
  $('#easyGame').hide();
  $('#hardGame').hide();
  generateTurtles();
  generateLogs();
  generateLeftCars();
  generateRightTrucks();
}

Which can actually be further refactored to...
$('.game-difficulty-opt').on('click', function(e) {
  pace = e.target.value.toLowerCase() === 'easy' ? true : false

  gameStartButtonPage();
});

function gameStartButtonPage () {
  $('#game').show();
  $('#startButton, #logo, .game-difficulty-opt').hide();   //should probably put this string into a variable
  generateTurtles();
  generateLogs();
  generateLeftCars();
  generateRightTrucks();
}

Now we no longer need this chunk of code in any of the functions to generate moving obstacles (which was repeated 4 times):
let pace;

if (isEasy) {
  pace = 2;
} else if (isEasy === false) {
  pace = 4;
}

If we remove the duplicate logic into one helper function, it might look something like this:
function generateLillypads() {
  lillypadArray = generateObstacles({
    x: 30, y: 25, w: 45, h: 30, count: 5, ObstacleType: Lillypad, add: true
  })

  return lillypadArray;
}

function generateTurtles() {
  turtleArray = generateObstacles({
    x: 0, y: 65, w: 80, h: 30, count: 6, ObstacleType: Turtle, add: true
  })
  return turtleArray;
}

function generateLogs() {
  logArray = generateObstacles({
    x: 900, y: 110, w: 85, h: 30, count: 6, ObstacleType: Log, add: true
  })

  return logArray;
}

function generateLeftCars() {
  carArrayLeft = generateObstacles({
    x: 900, y: 245, w: 100, h: 40, count: 4, ObstacleType: Car, add: false
  })

  return carArrayLeft;
}

function generateRightTrucks() {
  truckArray = generateObstacles({
    x: 0, y: 335, w: 100, h: 40, count: 4, ObstacleType: Truck, add: true
  })

  return truckArray;
}

function generateObstacles(data) {
  const { y, w, h, count, ObstacleType, add } = data
  let { x } = data
  const obstacleArray = []
  const obstaclePace = ObstacleType !== Lillypad ? pace : null

  for (let i = 0; i < count; i++) {
    let obstacle = new ObstacleType(x, y, w, h, obstaclePace)

    add ? x += 200 : x -= 200

    obstacleArray.push(obstacle)
  }

  return obstacleArray
}

With one helper function, we could now extract that from the larger game.js file, test it robustly and ensure that it works instead of having to test each separate function.
Duplicate Code / Lazy Class

What is the only difference between these classes?
class Turtle  extends MovingObstacle {
  constructor (x, y, width, height, pace, image) {
    super (x, y, width, height, pace);
    this.image = image;
  }
}

class Truck extends MovingObstacle {
  constructor (x, y, width, height, pace, image) {
    super (x, y, width, height, pace, image);
    this.image = image;
  }
}

class Log extends MovingObstacle {
  constructor (x, y, width, height, pace, image) {
    super (x, y, width, height, pace, image);
    this.image = image;
  }
}

class Car extends MovingObstacle {
  constructor (x, y, width, height, pace, image) {
    super (x, y, width, height, pace, image);
    this.image = image;
  }
}

There is really no reason to create so many classes with the exact same build. Instead, use a single constructor and pass through the differing images for each type. Ultimately you don't even need to extend from the MovingObstacle class, you can just use it as your template for each new instance. For example:
class MovingObstacle extends Square {
  constructor(x, y, width, height, pace, color, image) {
    super(x, y, width, height);
    this.pace = pace;
    this.color = color;
    this.image = image;
  }
}

const car = new MovingObstacle(10, 10, 20, 20, 4, blue, 'images/log.png')
const log = new MovingObstacle(10, 10, 20, 20, 4, blue, 'images/log.png')
const truck = new MovingObstacle(10, 10, 20, 20, 4, blue, 'images/log.png')
const turtle = new MovingObstacle(10, 10, 20, 20, 4, blue, 'images/log.png')


Long Method / Contrived Complexity

Sometimes figuring out what to refactor can simply be a matter of looking for large chunks of code. If a function gets too large, it's typically better to break out chunks into smaller, simpler functions. For example, look at this requestAnimationFrame function:
requestAnimationFrame(function gameloop() {

  if (currentGameState === 'game-on') {
    context.clearRect(0, 0, canvas.width, canvas.height);

    water.draw(context);
    safeLanding.draw(context);
    safeLanding2.draw(context);

    generatingLillypads.forEach(function(lillypad) {
      context.drawImage(lillypadImage, lillypad.x, lillypad.y, lillypad.width, lillypad.height);
    });

    logArray.forEach(function(log) {
      log.movLeft();
      context.drawImage(logImage, log.x, log.y, log.width, log.height);
    });

    turtleArray.forEach(function(turtle) {
      turtle.movRight();
      context.drawImage(turtleImage, turtle.x, turtle.y, turtle.width, turtle.height);
    });

    carArrayLeft.forEach(function(car) {
      car.movLeft();
      context.drawImage(carImage, car.x, car.y, car.width, car.height);
    });

    truckArray.forEach(function(truck) {
      truck.movRight();
      context.drawImage(truckImage, truck.x, truck.y, truck.width, truck.height);
    });

    context.font = "40px Squada One";
    context.fillStyle = 'rgba(0, 255, 0, 1)';
    context.fillText('Score:', 30, 490);
    context.fillText(newScore.points, 140, 490);
    context.drawImage(logo, 330, 445, 250, 70);
    context.fillText('Lives:', 740, 490);
    context.fillText(playerFrog.lives, 850, 490);

    playerFrog.drawImage(context);

    playerFrog.collisionDetection(canvas, logArray, turtleArray, carArrayLeft, lillypadArray, truckArray, resetGame, newScore);

    winner();
  }
  requestAnimationFrame(gameloop);
});

With over 30 lines of code, it's a perfect candidate for refactoring. Not to mention it's a key part of our game so we should be able to better understand what it's doing with a fresh look. Let's first think about what responsibility this function holds:

draw game board / background
clear the canvas each frame
draw player
set collision detection for player and obstacles
create/draw obstacles and give them instructions to move
fill score and lives text

With these broken out, we can now use it as a guide for how we'll refactor by breaking out smaller functions to handle these pieces. When we're done, it'll look something like this:
requestAnimationFrame(function gameloop() {

  if (currentGameState === 'game-on') {
    context.clearRect(0, 0, canvas.width, canvas.height);


    winner();
  }
  
  requestAnimationFrame(gameloop);
});

Much shorter! Perhaps we want to break it out even further, we can do that by breaking out functions into a more organized fashion such as:
function drawGame() {
  drawPlayer();
  drawGameBoard();
  drawObstacles();
  drawFooter();
}

Then we could call ONE single function in our animation loop by creating a high-level function to run the game:
function runGame() {
  context.clearRect(0, 0, canvas.width, canvas.height);
  drawGame();
  drawPlayer()
  winner();
}

Now our animation loop looks very clean:
requestAnimationFrame(function gameloop() {

  if (currentGameState === 'game-on') {
    runGame()
  }

  requestAnimationFrame(gameloop);
});

Too Many Parameters / Feature Envy / Inappropriate Intimacy:

While there is no set rule for the number of parameters a function should have...this is TOO MANY!
playerFrog.collisionDetection(canvas, logArray, turtleArray, carArrayLeft, lillypadArray, truckArray, resetGame, newScore);

Not to mention this player object knows WAY TOO MUCH about the game. If we think about OOP best-practices and the Law of Demeter (principle of least knowledge), a frog should only know about itself. It should not know about a game of any kind or how to reset it. Perhaps the instance of a game should know if two pieces within it are colliding...So to refactor, we could...

create a Game class that knows about itself and the pieces invovled
create a collision detection helper function that can assess when any two things are colliding and return true or false

// example helper function

function detectCollision(frog, obstacle) {
  let bottomOfObstacle = false;
  let leftSideOfObstacle = false;
  let rightSideOfObstacle = false;
  let topOfObstacle = false;
  
  if (frog.y <= obstacle.y + obstacle.height) {
    bottomOfObstacle = true;
  }
  if (frog.x + frog.width >= obstacle.x) {
    leftSideOfObstacle = true;
  }
  if (frog.x <= obstacle.x + obstacle.width) {
    rightSideOfObstacle = true;
  }
  if (frog.y + frog.height >= obstacle.y) {
    topOfObstacle = true;
  }
  if (bottomOfObstacle && leftSideOfObstacle && rightSideOfObstacle && topOfObstacle) {
    return true
  }

  return false
}

Then whenever we have an instance of a game which will have both a frog and obstacles, we can use this helper to first assess if the frog is colliding with something, then take action (such as resetGame or newScore. We have now reduced a lot of repetitive code and can likely remove this method altogether since the logic would reside within the instance of the game instead of the frog.
Exercise Time!

Get together with your GameTime partner(s) and...

Identify one or more chunks of code that could use refactoring
Paste the chunk(s) of code as part of a thread to my comment in slack
Include the steps you will take to refactor
Then, we will have each team present so we can talk through various strategies.

Finally, go refactor! Don't forget to rely on any tests you have to ensure the functionality persists. You can also write tests before refactoring if you have a good enough idea of how you will be refactoring it.
Feel free to start with the easy ones:
* debuggers
* left over console.log()
* //commented out code
* dead white space