Problem with array – JavaScript – SitePoint Forums

Alright, let’s take a look.

Load the script from the end of the body instead of in the head

The script code is being loaded into the document header.


    
    
    
    Arrays
    
    


It really shouldn’t be done these days. You are pretty limited to load it from there.

Instead, the script should be loaded from the end of the body element instead. For instance:


  
  


This way the script can more easily work with the page elements when loading the page.

Use JavaScript event listeners instead of inline event attributes

Here is the HTML code with the inline event attribute.

    

This was the original technique used when JavaScript was invented, before other, more improved techniques appeared. The inline event attribute really shouldn’t be used these days, because it mixes up JavaScript and HTML (which is a sin), and should be done as JavaScript code instead.

For instance:

    
    
const addBrandButton = document.querySelector("#elementos");
addBrandButton.addEventListener("click", function addBrandHandler(evt) {
  const el = document.getElementById('elemento');
  data(el.value);
});

This data function name could also be more appropriately named addToArray() instead. The intent is that someone looking at just this little snippet of code will have a fair idea of ​​what’s going to happen without having to go and investigate what the data() function is doing.

Let the variables speak for themselves

Currently you are using an array called things.

let things = [];

This name of things is far too generic a term. If you don’t know what kinds of things are going to be in the array, then an appropriate generic name for an array is items.

Since you’re limiting the number of selected brands, you’d better call it Featured Brands instead.

    // if (things.push(array1) < 4) {
    if (selectedBrands.push(array1) < 4) {
        // console.log(things);
        console.log(selectedBrands);
    }

    // if (things.length > 3) {
    if (selectedBrands.length > 3) {
        alert("You can't add more data")
        // let things1 = things.pop()
        let things1 = selectedBrands.pop()
        // let newBrands = brands.concat(things)
        let newBrands = brands.concat(selectedBrands)

By the way, don’t worry about converting the code to English. Most of us here are quite capable of using whatever language you prefer in code. Even though the language may change, the programming concepts remain consistent across all spoken languages.

Use function parameters

In addBrandHandler function we pass the value as argument, so in data function we have to use parameter to access this value.

// function data(array1) {
function data(array1) {
    // let array1 = document.getElementById('data1').value.toLowerCase();

This is also a good time to use a more meaningful name than array1, or add, and so a branded function parameter name seems to be more appropriate here.

// function data(array1) {
function data(brand) {
    // if (selectedBrands.push(array1) < 4) {
    if (selectedBrands.push(brand) < 4) {
        console.log(selectedBrands);
    }

Avoid strange magic whenever possible

The next line does some weird things.

    if (selectedBrands.push(brand) < 4) {

Not only does it add a mark to an array, but it gets a length from the array and compares the size of that length. It also leaves some puzzling questions, such as does the push tell you how many elements there were in the array before the new element was added? Or does the push return the new total size of the array?

That’s a lot going on in what should be a simple comparison.

Move the push out of the if statement, so you can later check the size of the array more clearly.

    // if (selectedBrands.push(brand) < 4) {
    selectedBrands.push(brand);
    if (selectedBrands.length < 4) {

When trying to figure out what is happening with the following code:

    if (selectedBrands.length < 4) {
        console.log(selectedBrands);
    }

    if (selectedBrands.length > 3) {
      ...
    }

I find that I always mentally convert less than 4 to less than or equal to 3, and having both 3 and 4 with a mix of comparisons can quickly get confusing.

We can make this more explicit by using

    // if (selectedBrands.length < 4) {
    if (selectedBrands.length <= 3) {
      ...
    }
    if (selectedBrands.length > 3) {
        alert("You can't add more data")
        let things1 = selectedBrands.pop()
      ...
    }

Avoid adding and then deleting the same information from tables

The existing code adds a mark to the array, and if the array is then too large, it removes that same element from the array.

   selectedBrands.push(brand);
    if (selectedBrands.length <= 3) {
        console.log(selectedBrands);
    } else {
        alert("You can't add more data")
        let things1 = selectedBrands.pop()

Instead of doing this dance with the array, it’s better to check if the array is too big and exit the function, instead of adding the item to the array and then deleting it again.

We can do this by moving the > 3 if the statement at the top of the function and adjusting it to be >= 3 (as happens before the element is pushed onto the array) and at the bottom we move the push into the if statement, so we can use a consistent array value of 3 when checking the length of the array.

    // if (selectedBrands.length > 3) {
    if (selectedBrands.length >= 3) {
        alert("You can't add more data")
        // let things1 = selectedBrands.pop()
        ...
    }
    // selectedBrands.push(brand);
    // if (selectedBrands.length <= 3) {
    if (selectedBrands.length < 3) {
        selectedBrands.push(brand);
        console.log(selectedBrands);
    }

Even better than that, exit the function at the end of the first if statement. This way, the second if statement won’t be necessary at all.

    if (selectedBrands.length >= 3) {
        alert("You can't add more data")
        ...
        return;
    }
    // if (selectedBrands.length < 3) {
        selectedBrands.push(brand);
        console.log(selectedBrands);
    // }

Summary

Other improvements need to be made, such as trying to always use const instead of let and extracting a few functions from the if statement, but that seems like a good place to leave things for now.

let brands = ['Samsung', 'Apple', 'Xiaomi', 'LG', 'Oppo']
let selectedBrands = [];

function data(brand) {
    if (selectedBrands.length >= 3) {
        alert("You can't add more data")
        let newBrands = brands.concat(selectedBrands)
        let array2 = newBrands.filter(brand => brand.length > 4)
        let array3 = array2.slice(0, 1)
        console.log(things1)
        console.log(newBrands)
        console.log(array2);
        console.log(array3);
        return;
    }
    selectedBrands.push(brand);
    console.log(selectedBrands);
}

const addBrandButton = document.querySelector("#elementos");
addBrandButton.addEventListener("click", function addBrandHandler(evt) {
  const el = document.getElementById('elemento');
  data(el.value);
});

Updated code for what we have at this point is available at https://jsfiddle.net/rm0j487a/

James S. Joseph