3
\$\begingroup\$

Can you please review my code and suggest why it may or may not be a professional looking code? I am just starting out as a programmer I am able to write code but I don't really know where I am right and where I am wrong.

Also, I have heard that using the MVC approach in code is a smart way. Please suggest what changes this code would have if it were to comply with the MVC approach.

About the Code: This application is part of a cat scoring app. You score the cats by clicking on them. This way the most liked cat can be determined. This code does not currently store the score. So, as soon as the refresh button is clicked, the score is lost.

This app is also available on CodePen.

const imageBasePath = "https://raw.githubusercontent.com/smartcoder2/CatClickerApp/master/images/";
const imageNameArrary = [
  "tom.jpg",
  "jack.jpeg",
  "zoe.jpeg",
  "simba.jpg",
  "george.jpeg"
];
let catScore = [0, 0, 0, 0, 0]; // this keeps the score of each cat. index of array determines the cat
let htmlUpdate;
let ddl;
const imageVar = document.getElementById("cat-image");
const textVar = document.getElementById("show-click-value");

imageVar.addEventListener("click", incrementClickVar);

function incrementClickVar() {
  ddl = document.getElementById("select-cat");
  catScore[ddl.selectedIndex]++;
  htmlUpdate =
    catScore[ddl.selectedIndex] == 0 ?
    "zero" :
    catScore[ddl.selectedIndex];
  textVar.innerHTML = htmlUpdate;
  //
}

function validate() {
  ddl = document.getElementById("select-cat");
  htmlUpdate =
    catScore[ddl.selectedIndex] == 0 ?
    "zero" :
    catScore[ddl.selectedIndex];
  textVar.innerHTML = htmlUpdate;
  let selectedValue = ddl.options[ddl.selectedIndex].value;
  imageVar.src = imageBasePath + imageNameArrary[ddl.selectedIndex];
}
.outer-box {
  height: 100vh;
  display: grid;
  grid-template-columns: 1fr 1fr 1fr;
  grid-template-rows: 1fr 1fr 1fr;
  grid-gap: 2vw;
  align-items: center;
}

.outer-box>div,
img {
  max-width: 25vw;
  min-height: 10vh;
  max-height: 44vh;
  justify-self: center;
}

.outer-box>div {
  text-align: center;
}

#show-click-value {
  font-size: 6vh;
}
<html>

<head>
  <link rel="stylesheet" href="styles\style.css" />
</head>

<body>
  <div class="outer-box">
    <div class="item1"></div>
    <div class="item2">
      <label class="cat-label" for="select-cat">Select a cat</label>
      <select id="select-cat" name="cats" onchange="validate()">
        <option value="Tom">Tom</option>
        <option value="Jack">Jack</option>
        <option value="Zoe">Zoe</option>
        <option value="Simba">Simba</option>
        <option value="George">George</option>
      </select>
      <br />
    </div>
    <div class="item3"></div>
    <div class="item4"></div>
    <img id="cat-image" src="https://raw.githubusercontent.com/smartcoder2/CatClickerApp/master/images/tom.jpg" alt="image not loaded" />
    <div class="item6"></div>
    <div class="item7"></div>
    <div id="show-click-value">zero</div>
    <div class="item9"></div>
  </div>
  <!-- srr-to-do-later: position the image and other elements properly -->

</body>

</html>

\$\endgroup\$
  • 1
    \$\begingroup\$ Lovely cats. The MVC approach is not aimed at simple interactions like this, it would be overkill. It is used in more complex applications with lots of user interactions and a bigger database in the background. \$\endgroup\$ – KIKO Software Jun 20 at 8:24
  • \$\begingroup\$ What is your code supposed to do? \$\endgroup\$ – IEatBagels Jun 21 at 14:22
  • 1
    \$\begingroup\$ @IEatBagels - you score the cats by clicking on them. This way the most liked cat can be determined. This code is not complete because I am not storing the score and as soon as the refresh button is clicked the score is lost. \$\endgroup\$ – Deski Rey Jun 21 at 15:16
  • 1
    \$\begingroup\$ @DeskiRey You should edit your question to include a description of what your code currently does. \$\endgroup\$ – frobinsonj Jun 21 at 15:25
4
\$\begingroup\$

Group cat data together in object, have a single source.

You have two arrays and an element, all joined together by a common index. I would move things around so all the details are coming from a single source. This means if you want to add a cat, you add it in one place. It also allows for an easy upgrade if you wanted to start storing this data somewhere that is not hard-coded.

I have created a new cats array. This contains all the cats and their data:

const cats = [{
    name: "Tom",
    image: "tom.jpg",
    score: 0
}];

For now we will stick with hard-coded data, however this could just as easily be a response from an API. For example:

fetch('https://example.com/cats.json')
  .then(function (response) {
    return response.json();
  })
  .then(function (cats) {
    //... do something with the cats
  });

The cats array stores all the details we need to populate our dropdown:

const catSelect = document.getElementById("cat-select");

cats.forEach(function(cat, index) {
    const option = document.createElement("option");
    option.value = index;
    option.text = cat.name;
    catSelect.add(option);
});

Handling user interaction

You have right idea with storing document.getElementById("cat-select") in a variable, however, you are redefining it on every user interaction (ddl = document.getElementById("cat-select");). Instead, lets define some constants. I have also added a new variable to keep track of the currently selected cat.

const catImage = document.getElementById("cat-image");
const catScore = document.getElementById("cat-score");
const catSelect = document.getElementById("cat-select");

let selectedCat;

Now we can use these in our functions. I have modified your two functions and also added one new one:

/*
This simple function is just to update the display of the score.
I thought it was nicer to put it in a function to avoid duplicating the code.
If you wanted to change it in the future, you would only have to change it in one place.
*/
function displayCatScore(score) {
    catScore.innerText = score == 0 ? "zero" : score;
}


/*
This simply updates the score of the currently selected cat and then displays it.
*/
function incrementSelectedCatScore() {
    displayCatScore(++selectedCat.score);
}


/*
This function updates `selectedCat` and displays that cat's score and image.
`cat` is the index of the cat that should be displayed.
*/
function displayCat(cat) {
    selectedCat = cats[cat];

    displayCatScore(selectedCat.score);

    catImage.src = imageBasePath + selectedCat.image;
    catImage.alt = selectedCat.name;
}

Now for event listeners - I have moved them from your HTML to the Javascript. It's easier when everything is kept together and in my opinion, it is just much cleaner.

catImage.addEventListener("click", incrementSelectedCatScore);

catSelect.addEventListener("change", function() {
    displayCat(this.value); // this.value will be the index of the selected cat
});

displayCat(0); // Display the first cat

Full working example:

function displayCatScore(score) {
  catScore.innerText = score == 0 ? "zero" : score;
}

function incrementSelectedCatScore() {
  displayCatScore(++selectedCat.score);
}

function displayCat(cat) {
  selectedCat = cats[cat];

  displayCatScore(selectedCat.score);

  catImage.src = imageBasePath + selectedCat.image;
  catImage.alt = selectedCat.name;
}

const imageBasePath = "https://raw.githubusercontent.com/smartcoder2/CatClickerApp/master/images/";

const cats = [{
    name: "Tom",
    image: "tom.jpg",
    score: 0
  },
  {
    name: "Jack",
    image: "jack.jpeg",
    score: 0
  },
  {
    name: "Zoe",
    image: "zoe.jpeg",
    score: 0
  },
  {
    name: "Simba",
    image: "simba.jpg",
    score: 0
  },
  {
    name: "George",
    image: "george.jpeg",
    score: 0
  }
];

const catImage = document.getElementById("cat-image");
const catScore = document.getElementById("cat-score");
const catSelect = document.getElementById("cat-select");

let selectedCat;

cats.forEach(function(cat, index) {
  const option = document.createElement("option");
  option.value = index;
  option.text = cat.name;
  catSelect.add(option);
});

catImage.addEventListener("click", incrementSelectedCatScore);

catSelect.addEventListener("change", function() {
  displayCat(this.value);
});

displayCat(0);
.outer-box {
  height: 100vh;
  display: grid;
  grid-template-columns: 1fr 1fr 1fr;
  grid-template-rows: 1fr 1fr 1fr;
  grid-gap: 2vw;
  align-items: center;
}

.outer-box>div,
img {
  max-width: 25vw;
  min-height: 10vh;
  max-height: 44vh;
  justify-self: center;
}

.outer-box>div {
  text-align: center;
}

#show-click-value {
  font-size: 6vh;
}
<html>

<head>
  <link rel="stylesheet" href="styles\style.css" />
</head>

<body>
  <div class="outer-box">
    <div class="item1"></div>
    <div class="item2">
      <label class="cat-label" for="cat-select">Select a cat</label>
      <select id="cat-select" name="cats"></select>
      <br />
    </div>
    <div class="item3"></div>
    <div class="item4"></div>
    <img id="cat-image">
    <div class="item6"></div>
    <div class="item7"></div>
    <div id="cat-score"></div>
    <div class="item9"></div>
  </div>
  <!-- srr-to-do-later: position the image and other elements properly -->

</body>

</html>


If you have any questions or would like any further clarification, please let me know and I will be happy to help :)

\$\endgroup\$
  • \$\begingroup\$ wow ! really grateful for this help. I'm still trying to absorb all the suggestions, but this is definitely going to improve my approach towards coding. thanks again, i might get back with some questions once i complete with implementing these suggestions. \$\endgroup\$ – Deski Rey Jun 21 at 14:20
  • \$\begingroup\$ And the format of the first callback function to the fetch() promise can be simplified to an arrow function: .then(response => response.json()) \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 21 at 16:03
3
\$\begingroup\$

About your HTML

You are missing the DOCTYPE. The first line should be:

<!DOCTYPE html>

It’s a good idea to declare the primary language of the document. In case of English:

<html lang="en">

It’s a good idea to specify the character encoding. The first element in the head element should be (in case of UTF-8):

<meta charset="utf-8" />

The title element is required. It belongs to the head element:

<title>Cat Scoring</title>

For linking your stylesheet, you probably mean styles/style.css instead of styles\style.css (the backslash is not allowed in the URL path, unless percent-encoded).

It’s not a big issue, but the br element is probably misused in this context. It should only be used for meaningful line breaks, not just for styling purposes. In your case, you should probably use CSS (e.g., margin-bottom).

Unless "image not loaded" is the content of the image, this doesn’t seem to be an appropriate alt value. The alt value should serve as an alternative (that conveys the meaning/purpose of the image) to users who can’t access it (e.g., because they are blind).

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