What is the best "practice" or form of code in javascript (example)

Question

I am currently doing CS50. And i feel a little lost with javascript, i don't know the best way to do things or to write "correct code". I feel that in C and in Python, the lines to do things were more clear. (Don't repeate code, make it legible, etc) In javascript, we can do things in many ways and i don't know what it's the correct.

For example i spent a few days on trivia, a problem that ask us to make buttons that turn red if the button is the incorrect answer and green if the answer is correct, i know that it's a good practice to leave all the javascript in one file different from the html, first i only use onclick in html to call a function that i implemented in the js file then i tried to change it to don't have any js code in the html page.

So i wanted to remove the onclick call, so, i put and addEventListener to the javascript on load and do all the functions there, with this, i want to check if the innerHtml of a button it's the same from the correct answer, but this code doesn't work, i think this code is the correct version because is really checking if the answer is correct

window.addEventListener("load", function () {

//Declaring first function

function checkAnswer(event) {

var buttonPressed = event.Target;

if (buttonPressed.innerHTML == 'Uruguay')

{

buttonPressed.style.background = "green";

}

else:

{

buttonPressed.style.background = "red";

}

var buttons = document.querySelectorAll('button');

buttons.forEach(b => b.addEventListener("click", checkAnswer(event)));

}

Because i was having trouble with this to function i ask a friend who uses javascript to look at it, and he say to me that there is an easy way, id all the buttons by correct or incorrect and if the user click the correct execute a function attached to that button to color the button green and if its red execute another:

const correctButton = document.getElementById("correctButton");

const Incorrectbuttons = document.querySelectorAll("#Incorrectbutton");

bcorrectButton.addEventListener("click", function(){
correctButton.style.background = "green";

});

Incorrectbuttons.forEach(btn => {

btn.addEventListener("click", function(){

btn.style.background = "red";

});

})

I feel that this code cheat because it actually don't check if the button is correct, if an user changes the inner html of the word in his browser, the button still turned green, in my code, i think that if the user change the inner html it wont change because the innerHtml don't match the "key".

I don't know if i am viewing it wrong becaused i am attached to other forms of thinking with other languages and i can do things "directly" without logic on javascript or my first function is completly wrong. Thanks!

Answer 1

Just to show you the beauty of a language as JavaScript - here's two possible approaches (out of many, really), one simple, and the other a little bit more advanced with dynamic templates generation - all out of some data model predefined in code:

1. Simpler solution:

Use data-* attribute on your parent element to store the correct answer string.

Simply place your buttons into HTML and assign the correct answer value into the parent element data-answer attribute:

const ELS_answer = document.querySelectorAll("[data-answer]");

ELS_answer.forEach(EL => {

  // Assign click handler to parent element with data-answer attribute
  EL.addEventListener("click", (ev) => {

    // Get the data attribute of the parent element
    const correct = ev.currentTarget.dataset.answer;

    // Get the text of the clicked button
    const answer = ev.target.closest("button").textContent;

    // Check if correct
    if (answer === correct) {
      alert("CORRECT! (PS: make button green here!)");
    } else {
      alert("Not correct :(");
    }
  });
  
});

<div data-answer="Five">
  <h3>What goes after "Four"?</h3>
  <button type="button">One</button>
  <button type="button">Five</button>
  <button type="button">Nine</button>
</div>

<div data-answer="nice">
  <h3>This site is pretty ____</h3>
  <button type="button">nice</button>
  <button type="button">boring</button>
  <button type="button">sweet</button>
</div>

<div data-answer="8">
  <h3>How many bits are in a single byte?</h3>
  <button type="button">4</button>
  <button type="button">8</button>
  <button type="button">16</button>
</div>

As you can see above, you're not polluting your JavaScript code logic with strings, and that code can work for an infinite number of trivia ("Multiple choice") questions and buttons.

2. Advanced solution

Keep in mind - data!

So how would you structure your data for the Part 1 (and even Part 2 "Free response") of the CS50 Lab? You have multiple buttons (therefore think of it as an Array) and you have only one correct answer.
And you also have types (currently two: Multiple choices and Free Response) therefore type, question, answers, correct can all be properties of each Object item inside the trivia Array.

Such can be modelled like:

const trivia = [

  { 
    // `type` will help you create specific HTML Elements markup (template)
    type: "multiple",
    // The question that goes into H3
    question: "What is the first word of the well known typesetting text?",
    // Loop this Array values to generate your buttons!
    answers: ["Ipsum", "Lorem", "Amet", "Sit", "Dolor"],
    // Here you can store the correct answer
    correct: "Lorem",
  },
  
  {
    type: "free",
    question: "What is the best community driven website for developers?",  
    answers: [], // will be populated by user!
    correct: "Stack Overflow",
  },

];

given the above you can iterate your trivia Array and generate HTML.

If the currently iterating trivia item type is "multiple" - generate <h3> and your buttons.
If the currently iterating trivia item type is "free" - generate <h3>, an <input type="text">, and a button.

// Trivia HTML templates functions:

const templates = {
  multiple(data) { /* GENETATE THE "MULTIPLE" TEMPLATE and append to DOM */ }, 
  free(data)     { /* GENETATE THE "FREE" TEMPLATE and append to DOM */ },
  // In the future you can add more template types here
};

// Iterate your trivia objects and generate HTML 
trivia.forEach((data) => {
  templates[data.type](data); // call a specific template generator!
});

Given the above you can have as many trivia questions as you like.
And in the future you can expand the above data for other interesting types.

To generate your buttons all you need to do is iterate the data.answers like:

const trivia = [
  { 
    type:     "multiple",
    question: "What is the first word of the well known typesetting text?",
    answers:  ["Ipsum", "Lorem", "Amet", "Sit", "Dolor"],
    correct:  "Lorem",
  },
  {
    type:     "free",
    question: "What is the best community driven website for developers?",  
    answers:  [],
    correct:  "Stack Overflow",
  },
];

// Utility functions
// Create a new Element tag with properties:
const newEL = (tag, props) => Object.assign(document.createElement(tag), props);

// Trivia HTML templates:
const templates = {

  multiple(data) {
  
    // Create the Heading
    const H3 = newEL("h3", {textContent: data.question});
    
    // Create the Buttons
    const BUTTONS = data.answers.reduce((DF, answer) => {
      // Create a single button
      const BUTTON = newEL("button", {
        type:"button",
        textContent: answer,
        onclick() {
          // check if answer is correct
          if (data.correct === this.textContent) {
            alert("CORRECT! PS: style your button green here");
          } else {
            alert("Not norrect");
          }
        }
      });
      DF.append(BUTTON); // Append every button to the DocumentFragment
      return DF;         // Return the DocumentFragment
    }, new DocumentFragment());
    
    // Finally, append everything to a specific DOM Element parent
    document.querySelector("#multiple").append(H3, BUTTONS);
  },
  
  free(data) {
    // TODO!
    // Similar as the above one
    // You can do it!!!
  }
};

// Iterate your trivia objects and generate HTML templates
trivia.forEach((data) => templates[data.type](data));

<div id="multiple"></div>

Regarding security / hacks

You're not in the phase to worry about security at this stage, for this Lab task. But it's nice you have that always in mind because it's extremely important!

Since the code (HTML, JS, Network requests, etc) that arrives to the browser can be spoofed or reverse-engeneered (like in the above Example 1. has the answers hardcoded in HTML; and Example 2. has them right in JS), in order to not allow a user to see the correct answers the proper approach would be to have the correct answers stored somewhere on your server (not visible to the user). Send the user selected answer to the server and check on server-side the correctness. Than the server should only respond to the browser with a response (i.e: "correct" / "wrong") so that the user interface can advance to the next question or give a final overall score. Even than, the server should never trust the user! Therefore you should implement a sort of user session using browser+backend technologies like i.e: a session cookie, and check if the data that arrives from the user matches the state in the database for that specific user (current question, correct questions count, timestamp, etc etc).
Not a topic or problem that has to be solved at this stage in the CS50 Lab. But if you want - nobody stops you to explore and implement a more robust and secure solution on your own!

Answer 2

Well, there are many wrongs in your code. However, you are on the correct track!

Something to keep in mind :

• You can declare your functions outside the page load event.
• When adding an event listener then you have to pass the function, not call it.

Try this :

//The load event is fired when the whole page has loaded
window.addEventListener("load", function() {

    //Get all buttons in the page
    let buttons = document.querySelectorAll('button');

    //Set a click event listener for each button
    //When the event occurs then it calls the checkAnswer event handler callback and passes the event to it
    //Notice that the callback function is passed and not directly called with "()"!
    buttons.forEach(b => b.addEventListener("click", checkAnswer));
});

//Check answer function
function checkAnswer(event) { // <- here we get the event and use it

    //Get the presses button
    var buttonPressed = event.target;

    //Check the content of the button
    if (buttonPressed.innerHTML == 'Uruguay') {
        //Change background to green
        buttonPressed.style.background = "green";
    } else {
        //Change background to red
        buttonPressed.style.background = "red";
    }
}

Edit :

Regarding the security aspect. In both codes provided (your's and your friend's), the user can manipulate the data in the HTML and get the correct answer. Such checks have to be server-sided where you tell your server to check the value sent by the user (the value of the clicked button) if it's correct or not. And then based on the server's answer you proceed on displaying if the chosen button/answer is correct or not.