ChoreDoors Code Review

[smissaertj]

I would appreciate it if someone could point out what is wrong with my isClicked() function and/or the if block in door1.onclick() ?

door1.onclick() doesn’t change the door1.src attribute after adding the if block.

const doorImage1 = document.getElementById('door1');
const doorImage2 = document.getElementById('door2');
const doorImage3 = document.getElementById('door3');

const imgPath = './img/';
const botDoorImg = imgPath + 'robot.svg';
const beachDoorImg = imgPath + 'beach.svg';
const spaceDoorImg = imgPath + 'space.svg';
const closedDoorImg = imgPath + 'closed_door.svg';

let numClosedDoors = 3;

let openDoor1;
let openDoor2;
let openDoor3;

function isClicked(door){
  if (door.src === closedDoorImg){
    return false;
  } else {
    return true;
  }
}

const playDoor = () => {
  numClosedDoors--;
  console.log(numClosedDoors);

  if (numClosedDoors === 0){
    //gameOver();
    alert('Game Over');
  }
}

const randomChoreDoorGenerator = () => {
  let choreDoor = Math.floor(Math.random() * numClosedDoors);

  if (choreDoor === 0) {
    openDoor1 = botDoorImg;
    openDoor2 = beachDoorImg;
    openDoor3 = spaceDoorImg;

  } else if (choreDoor === 1) {
    openDoor2 = botDoorImg;
    openDoor1 = beachDoorImg;
    openDoor3 = spaceDoorImg;

  } else if (choreDoor === 2){
    openDoor3 = botDoorImg;
    openDoor2 = beachDoorImg;
    openDoor1 = spaceDoorImg;
  }

}

door1.onclick = () => {
  if (!isClicked(door1)){
      door1.src = openDoor1;
      playDoor();
  }
}

door2.onclick = () => {
  door2.src = openDoor2;
  playDoor();
}

door3.onclick = () => {
  door3.src = openDoor3;
  playDoor();
}

randomChoreDoorGenerator();

[janneslohmeijer]

What is door1? Did you call them from the dom by using getElementById? You might want to look into that ;).

[smissaertj]

Thank you for your reply.

Unless I’m not understanding something, yes I did, kindly refer to the first line of the code:
const doorImage1 = document.getElementById('door1');

Chore Door project

The explanation for this came up a few weeks ago, and the conversation led to a ticket being submitted, and the course revised. I’ll have to dig around for that topic. Briefly, id="door1", causes the browser to create a global variable by that name. What gets assigned I wouldn’t know. I suspect it to be a nodelist, which is why we could access it even without caching the nodes. door1 indeed had a src attribute. I tracked down the conversation but as it is DM, have asked the other participants…

[janneslohmeijer]

True, but after that you are not calling doorImage1 anywhere in your code ;). Instead you are using the ids from the HTML and not the doorImage1, 2 and 3.

[janneslohmeijer]

Btw your image location is wrong. The images are not stored in the same folder where the code is. Instead it is stored outside of the codecademy website.

const botDoorPath = "https://s3.amazonaws.com/codecademy-content/projects/chore-door/images/robot.svg";
const beachDoorPath = "https://s3.amazonaws.com/codecademy-content/projects/chore-door/images/beach.svg";
const spaceDoorPath = "https://s3.amazonaws.com/codecademy-content/projects/chore-door/images/space.svg";
const closedDoorPath = "https://s3.amazonaws.com/codecademy-content/projects/chore-door/images/closed_door.svg"

[smissaertj]

This doesn’t make any sense The other doors without the if blocks are working.

door2.onclick = () => {
  door2.src = openDoor2;
  playDoor();
}

door3.onclick = () => {
  door3.src = openDoor3;
  playDoor();
}

Also, the paths are different from the instructions. I’ve saved the images locally and the code is running locally instead of on Code Academy . There’s nothing wrong with the image paths in the code.

[janneslohmeijer]

Is the console giving any errors? (I can’t answer in the next 2 hours due to work meeting).

[smissaertj]

Not at all unfortunately. See below screenshot.
The output you see in the console is console.log(numClosedDoors); in the playDoor() function.

[smissaertj]

Right, so for some reason my code does work in the Code Academy editor, but not locally.

Edit: Modifying the image paths to the Code Academy S3 bucket solves my issue locally, so the problem lies with the relative paths somewhere. I’m curious to find out what it is exactly. If I ever do, I’ll post it here

[mtf]

door2.onclick = () => { door2.src = openDoor2; playDoor(); } door3.onclick = () => { door3.src = openDoor3; playDoor(); }

Thoses two are still referring to the globals, and not doorImageN.

const imgPath = ‘./img/’;

If we can assume you’ve got the img folder in the same directory as index.html.

imgPath = "img/"

will work fine. It is rather moot, though.

botDoorImg = "img/robot.svg";

[smissaertj]

Thanks for the feedback.

Isn’t doorImageN a global variable as well? It’s declared in the global scope, no?

Even after taking your advice on the paths, the doors just don’t open when using local paths.
Could you please elaborate as to why this would be “moot” ?

[mtf]

Isn’t doorImageN a global variable as well? It’s declared in the global scope, no?

Yes it is declared in global scope, in this example, but it is less likely some other code will collide with them because they are not in the HTML as id. Given we declare them as constants, they cannot collide with anythng.

the doors just don’t open when using local paths.

Do you have the images stored in the img folder?

why this would be “moot” ?

The variable you are assigning img/ to is longer than the actual path name. Simpler just to use it and not take up memory with a superfluous variable.

[smissaertj]

Thank you for your valuable input Sir, I now understand what you were pointing out regarding my first point on global scope.

As pointed out in my previous posts, the doors open fine, the images change when I don’t use the if block in the .onclick functions. As soon as the if block is there, it doesn’t work anymore.
I understand this is probably out of scope for Code Academy (since I’m trying this locally), but it does intrigue me why it’s not working.

As for your last point, I actually followed your advice in another post, basically saying DRY:

let someImage = "path/to/image.jpg";
let someImage2 = "path/to/image2.jpg";

should better be:

let imagePath = "path/to/";
let someImage = imagePath + "image.jpg";
let someImage2 = imagePath + "image2.jpg";

[mtf]

I actually followed your advice in another post

That did look familiar… It would apply more to the long URLs in the track version. If the URL is a local path, then I would go with whatever is shortest, and not use a variable (which uses memory) if I didn’t have to. It’s not a deal breaker and it does help readability, to a certain degree.