Yesterday I asked a question about improving efficiency in my code. Today I have another question in the same spirit of trying to write less lines of code to acplish repetitive tasks.

I have the following code:

function myIntroductionText() {
    introPos.style.display = 'block';
    posOne.style.display = 'none';
    posTwo.style.display = 'none';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
    backButton.style.visibility = 'hidden';
}

function myPositionOne() {
    introPos.style.display = 'none';
    posOne.style.display = 'block';
    posTwo.style.display = 'none';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
    backButton.style.visibility = 'visible';
}

function myPositionTwo() {
    introPos.style.display = 'none';
    posOne.style.display = 'none';
    posTwo.style.display = 'block';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
}

The HTML looks something like this:

<p class="textContent" id="introductionText">Introduction Text Goes Here</p>
                <p class="textContent" id="position1">content1</p>
                <p class="textContent" id="position2">content2</p>
                <p class="textContent" id="position3">content3</p>

Each position (i.e. introPos, posOne, posTwo) also has a corresponding function that looks essentially the same as the function above, except it changes the display based on which position it is in.

I'm thinking that I could use a loop and/or an if/else statement to make this task more efficient. I tried by using getElementsByClassName('textContent'), which (I think) produced an array containing all of the elements with that class. According to the console.log is contains [p#introductionText.textContent, p#position1.textContent, so on and so on...]. So, I wrote the following code to try to loop through it:

var blanks = document.getElementsByClassName("textContent") // this creates the array that I mentioned
for (item in blanks) {
            if (blanks[0] === introductionText.textContent) {
                blanks[0].style.display = 'block';
            } else {
                blanks[item].style.display = 'block';
                }
        }

I tried using p#introductionText.textContent but that returned an error. I'm very new to JavaScript so I fully recognize that I could be doing something very silly here, but any help would be appreciated.

EDIT: The error message says Uncaught SyntaxError: Unexpected tocken ILLEGAL

I should also add that my goal is to have only one position be visible at each time. I have a "Back" and "Next" button that allows users to go from posOne to posTwo, to posThree, and so on. So, in addition to making posTwo visible, I also need to make posOne and/or posThree not visible.

Thanks!

Yesterday I asked a question about improving efficiency in my code. Today I have another question in the same spirit of trying to write less lines of code to acplish repetitive tasks.

I have the following code:

function myIntroductionText() {
    introPos.style.display = 'block';
    posOne.style.display = 'none';
    posTwo.style.display = 'none';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
    backButton.style.visibility = 'hidden';
}

function myPositionOne() {
    introPos.style.display = 'none';
    posOne.style.display = 'block';
    posTwo.style.display = 'none';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
    backButton.style.visibility = 'visible';
}

function myPositionTwo() {
    introPos.style.display = 'none';
    posOne.style.display = 'none';
    posTwo.style.display = 'block';
    posThree.style.display = 'none';
    posFour.style.display = 'none';
    posFive.style.display = 'none';
    posSix.style.display = 'none';
    posSeven.style.display = 'none';
    posEight.style.display = 'none';
    posNine.style.display = 'none';
    posTen.style.display = 'none';
    posEleven.style.display = 'none';
}

The HTML looks something like this:

<p class="textContent" id="introductionText">Introduction Text Goes Here</p>
                <p class="textContent" id="position1">content1</p>
                <p class="textContent" id="position2">content2</p>
                <p class="textContent" id="position3">content3</p>

Each position (i.e. introPos, posOne, posTwo) also has a corresponding function that looks essentially the same as the function above, except it changes the display based on which position it is in.

I'm thinking that I could use a loop and/or an if/else statement to make this task more efficient. I tried by using getElementsByClassName('textContent'), which (I think) produced an array containing all of the elements with that class. According to the console.log is contains [p#introductionText.textContent, p#position1.textContent, so on and so on...]. So, I wrote the following code to try to loop through it:

var blanks = document.getElementsByClassName("textContent") // this creates the array that I mentioned
for (item in blanks) {
            if (blanks[0] === introductionText.textContent) {
                blanks[0].style.display = 'block';
            } else {
                blanks[item].style.display = 'block';
                }
        }

I tried using p#introductionText.textContent but that returned an error. I'm very new to JavaScript so I fully recognize that I could be doing something very silly here, but any help would be appreciated.

EDIT: The error message says Uncaught SyntaxError: Unexpected tocken ILLEGAL

I should also add that my goal is to have only one position be visible at each time. I have a "Back" and "Next" button that allows users to go from posOne to posTwo, to posThree, and so on. So, in addition to making posTwo visible, I also need to make posOne and/or posThree not visible.

Thanks!

• javascript
• function
• loops
• coding-efficiency

• What is the error message? – Teemu Commented Jan 22, 2016 at 15:32
• There's no need to reset all your elements display to none everytime. You only need to do it once. – MinusFour Commented Jan 22, 2016 at 15:41
• the error is your if? if (blanks[0].id === 'introductionText')? – Guillermo81 Commented Jan 22, 2016 at 15:41
• If I do if (blanks[0].id === 'introductionText') I get an error message stating "Uncaught TypeError: Cannot set property 'display' of undefined"? – user3822485 Commented Jan 22, 2016 at 15:48
• But blanks[0] is an element, introductionText.textContent is supposed to be a string??? Use for loop to iterate a collection, not for..in, use document.getElementById to get an element by id, don't make direct references. Also, what exactly is the illegal token? There isn't any illegal characters in the example you've proved. – Teemu Commented Jan 22, 2016 at 15:51

3 Answers 3

The first thing is moving all those Javascript style expressions to CSS:

#introPos, 
#posOne,
#posTwo, 
#posThree, 
#posFour, 
#posFive, 
#posSix, 
#posSeven, 
#posEight, 
#posNine, 
#posTen, 
#posEleven {
    display: none;
}

Or even shorter

#introductionText>.textContent {
    display: none;
}

This would enable you to shorten each function considerably:

function myPositionOne() {
    posOne.style.display = 'block';
    backButton.style.visibility = 'visible';
}

Instead of setting each style via JS again and again, you'd simply set those that change.

The next step would be to rewrite all those functions into one that accepts a parameter which element you are targeting:

function myPosition(pos) {
    var parent = document.getElementById("text-container");
    var children = parent.getElementsByClassName("textContent");

     var element;
    // first hide all <p class="textContent"> children
    for (var i = 0; i < children.length; i++) {
        children[i].style.display = 'none';
        if (i == pos) {
          element = children[i];
        }
    }

    // then show the right one

    if (element) {
        element.style.display = 'block';
    }

    // show or hide the back button depending on which child we are dealing with
    if (pos > 0) {
        document.getElementById("backButton").style.visibility = 'visible';
    } else {
        document.getElementById("backButton").style.visibility = 'hidden';
    }
    if (pos >= children.length-1) {
        document.getElementById("nextButton").style.visibility = 'hidden';
    } else {
            document.getElementById("nextButton").style.visibility = 'visible';
    }
}

This sets only the child number #pos visible and adjusts the visibility of the back button (assuming the back button has the ID "backButton").

Maybe this: All paragraphs also have the class "textContent". Make this display none and display the correct paragraph via given paragraph-id:

function myFunction(classDisplay) {

   var elems = document.getElementsByClassName('textContent');
   for (var i=0;i<elems.length;i+=1){
      elems[i].style.display = 'none';
   }
   document.getElementById(classDisplay).style.display = "block";
}

The following will hide all but position 2:

    myFunction("position2");

I don't know about the back-button, this is always be visible?

EDIT: I've tested this and corrected the code.

If you use JQuery, you can also use the following instead of the for loop:

$('.textContent').css('display'​​​​​​​​​​​​​​​​​​​​​​​​​​​,'none');​​​​​​

In newer versions of JavaScript you can use:

Array.from(document.getElementsByClassName('myclass')).forEach((item) => {
    item.style.backgroundColor = "blue";
 })