Okay, let’s have 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="https://www.sitepoint.com/community/t/problem-with-array/ejercicio1.js"></script>
<link rel="icon" type="image/x-icon" href="http://www.sitepoint.com/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="https://www.sitepoint.com/community/t/problem-with-array/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/
