Problem with array

JavaScript
1

I have to make an array exercise, i have to make 2 arrays, one with 5 elements and the other one empty.
I need to make the empty one get elements with an input in html. After that i need to make sure that the empty array can only have 3 elements, and show an alert if you try to put more. After all that i need to use concat to link the first array of 5 elements with this new one. I’m unable to do anything. I tried doing the input but i only get to save 1 element.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Arrays</title>
    <script src="ejercicio1.js"></script>
    <link rel="icon" type="image/x-icon" href="/HTML_CSS/imagenes/contra.png">

</head>
<body>
        <form>
                <input type="text" name="" id="elemento">
                <input onclick="añadir()" id="elementos" type="button" value="Añadir al array">


        </form>
    


    
</body>
</html>



function añadir(){
    var marcas = ['Samsung','Apple', 'LG', 'Xiaomi', 'Oppo'];
    var cosas = [];
    let agregar = document.getElementById('elemento').value.toLowerCase();

    cosas.unshift(agregar);

    console.log(cosas);

   if(cosas >3){
    alert('No se pueden añadir mas elementos')
   }else if(cosas>3){
    var concatenado = cosas.concat(marcas);
    console.log(concatenado);
   }

   var ultimo = concatenado.pop();
   console.log(ultimo);

   var nuevoArray = concatenado.filter(array => array.length <=4);
   console.log(nuevoArray);

   var array2 = nuevoArray.splice(2, 'Movil');

   console.log(array2)

   var array3 = array3.reverse;

   console.log(array3)


    
}
2

Check your browser’s console and you will see the error:

Uncaught TypeError: concatenado is undefined

This is cause because the concatenado variable is out of scope to your main program, as it was declared inside an if statement.

This is the first thing you need to fix.

1 Like
3

First of all. Please do not use local language in a source code. Start directly programming only in English. Might be a little be more work at the beginning but at the end when you want to (or maybe have to because its your work) share your code with others, it is needed.

You always redefine your variables when you call the add function. So every time you click on the button the variables are reseted and so you only have 1 item in the reason array. Yes need to define the variables outside of the function

1 Like
4

I am not quiet sure if you are right. I do no longer use “var” but as I remember var has the advantage that its scope is always the whole function. With “let” you are right.

3 Likes
5

Man, you are right. I haven’t used var for a while and was thinking of let / const.

TBH, I was caught between just giving the OP the answer and providing them with a hint, as this seems to be a homework exercise.

I should have paid more attention. Nice catch :slight_smile:

2 Likes
7

Thank you. Will have that in mind and try to program in english as it will make it easier for me and for when i ask questions

8

cosas is an array, it cant be compared directly to a number this way. You perhaps meant to get the length of the array for comparison.

You cant else if with the same test and expect it to ever work; if it passed the first time, it wont get to the else. if it failed the first time, it will fail again.

9

You should use const and let instead of var. Always use const unless you are sure that the value will change, if value will change use let!

Move your var marcas and var cosas outside of the anadir function and refactor your code.

const marcas = ["Samsung", "Apple", "LG", "Xiaomi", "Oppo"];
const cosas = [];

function añadir() {
  const agregar = document.getElementById("elemento").value.toLowerCase();

  if (cosas.length < 3) {
    const newCosas = cosas.unshift(agregar);
    const newMarcas = marcas.concat(newCosas);
    return newMarcas;
  }
  alert("No se pueden añadir mas elementos");
}
10

Thank you. I changed all my code to english, and made changes. Now the array it’s taking elements, not allowing more than 3 and giving and alert if you try to put more than 3. Everything is working, but i want to remove 2 elements from the array, then add 1 more and show it in reverse order, starting from z to a.
I tried the splice method to remove but it’s not working, and i don’t know how to reverse the order.

And i wanted to ask you something you said to use const or let instead of var. In this case should i use a let because the array is going to change or can i use a const?


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

function data(){

    let array1 = document.getElementById('data1').value.toLowerCase();
    if(things.push(array1) < 4 ){
    
    console.log(things);
} 

    if(things.length > 3){
        alert("You can't add more data")
        let things1 = things.pop()
        let newBrands = brands.concat(things)
        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);
        

        
    }

    
    
    
}
11

Yes, you can use const in brands and things here.

“const” is a little confusing word.

The const does not define the constant of the array; instead, a reference to an array is defined by the constant.

So elements of a constant array can still be changed,

To remove and add element + reverse order here is a simple example to help you to go

let arrayOriginal = ["Samsung", "Apple", "Xiaomi", "LG", "Oppo"];

const newArray = arrayOriginal.slice(0, arrayOriginal.length - 2).concat("Itel").reverse()

console.log(newArray); // [ 'Itel', 'Xiaomi', 'Apple', 'Samsung' ]
12

Okay, let’s take a look.

Load script from end of body instead of in the head

The scripting code is currently being loaded in the head of the document.

<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Arrays</title>
    <script src="ejercicio1.js"></script>
    <link rel="icon" type="image/x-icon" href="/HTML_CSS/imagenes/contra.png">

</head>

That really shouldn’t be done these days. You are quite restricted loading it from there.

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

<body>
  <!-- rest of page here  -->
  <script src="ejercicio1.js"></script>
</body>
</html>

That way, the script can more easily work with elements on the page as the page loads.

Use JavaScript event listeners instead of Inline event attributes

Here is the HTML code with the inline event attribute.

    <input onclick="añadir()" id="elementos" type="button" value="Añadir al array">

That was the original technique used when JavaScript was invented, before other more improved techniques came along. The inline event attribute really shouldn’t be used these days, as it’s mixing JavaScript and HTML together (which is a sin), and really should be done as JavaScript code instead.

For example:

    <!--<input onclick="añadir()" id="elementos" type="button" value="Añadir al array">-->
    <input id="elementos" type="button" value="Añadir al array">

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

That data function name might also be more suitably named addToArray() instead. The whole intention is so that someone just looking at that small snippet of code, has a fair idea about what is going to happen without needing to go and investigate just what the data() function does.

Let variables speak for themselves

Currently you are using an array called things.

let things = [];

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

Given that you are limiting the number of brands that are selected, you would be better placed calling it selectedBrands 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 fully capable of using whichever language you prefer in the code. Even though the language might change, the programming concepts remain consistent across spoken languages.

Use function parameters

In the addBrandHandler function we are passing the value as an argument, so in the data function we should use a parameter to access that 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 function parameter name of brand seems to be more suitable here.

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

Avoid weird magic where possible

The following line does some strange things.

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

Not only is it adding a brand to an array, it is getting a length of the array, and is comparing the size of that length. It also leaves some unsettling questions, such as does the push tell you how many items there were in the array before the new item was added? Or does the push return the total new size of the array?

That’s a lot of things to be happening in what should be a simple comparison.

Move the push out of the if statement, so that you can then check the size of the array with greater clarity.

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

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

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

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

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

We can make that more explicit, by using <= with 3, which helps to ensure that the same number is used and that the code is easier to understand.

    // 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 removing the same info from arrays

The existing code adds a brand to the array, and if the array is then too large it removes that same item 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 that dance with the array, it’s better to check if the array is too large and exit the function, instead of adding the item to the array then removing it again.

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

    // 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 is to return out of the function at the end of the first if statement. That way the second if statement won’t be needed 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

There are more improvements that should be done, such as by trying to always use const instead of let, and extracting a couple of functions from the if statement, but this seems to be 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);
});

The updated code for what we have at this stage is up at https://jsfiddle.net/rm0j487a/

13

Thank you. After seeing this code i know that i have much to learn but it really helps to know how a professional code would look and the steps needed to get there.

1 Like
14

Thank you it’s working now with the tips you gave me

15

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.