Money converter

I’m trying to do a money converter, with only a few currencies and putting the exchange rate by hand because y don’t know yet how to link a real currency exchange.

So i made this code, but it’s not working, i know that the event.listner is wrong, but i don`t know how to make it work. I tried to see if the currency exchange worked using an onclick function on the button but i get the same result always, it’s not changing currency, it gives the message “We can’t change that currency”

This is the code

<!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>Convertidor</title>
    <!-- CSS only -->
    <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.2.1/dist/css/bootstrap.min.css" 
    rel="stylesheet" 
    integrity="sha384-iYQeCzEYFbKjA/T2uDLTpkwGzCiq6soy8tYaI1GyVh/UjpbCx/TYkiZhlZB6+fzT" crossorigin="anonymous">
    <link rel="stylesheet" href="moneda.css">
</head>
<body>
<form>
    <label for="exampleDataList" class="form-label">First currency</label>
    <input class="inputMoney" list="money" id="money1" placeholder="€,$,£,">
    <datalist id="money">
      <option value="Euro">
      <option value="Dolar">
      <option value="Pound">
      
      
    </datalist>

    <label for="exampleDataList" class="form-label">Second Currency</label>
<input class="inputMoney" list="moneyConverted" id="money2" placeholder="€,$,£,">
<datalist id="moneyConverted">
     <option value="Euro">
     <option value="Dolar">
     <option value="Pound">
     
</datalist>

<br>

<label>Amount:</label>
<input type="text" id="amount" />
<button type="button" id="convert">Convert</button>
<p id="result"></p>
</form>

     <!-- JavaScript Bundle with Popper -->
     <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.2.1/dist/js/bootstrap.bundle.min.js" integrity="sha384-u1OknCvxWvY5kfmNBILK2hRnQC3Pr17a+RTT6rIHI7NnikvbZlHgTPOOmMi466C8" crossorigin="anonymous"></script>
    <script src="moneda.js"></script>
</body>
</html>
const money1 = document.getElementById('money1').value;
const money2 = document.getElementById('money2').value
const amount = document.getElementById('amount');
const converted = document.getElementById('result');

function change(){
    if(money1 === 'Euro' && money2 === 'Dolar'){
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05
        
    }else if(money1 === 'Euro' && money2 ==='Libra'){
        
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05
        
    }else if(money1 === 'Dolar' && money2 ==='Euro'){
        
    const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05
     
    
    }else if(money1 === 'Dolar' && money2 ==='Libra'){
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05

    }else if(money1 === 'Libra' && money2 ==='Euro'){
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05

    }else if(money1 === 'Libra' && money2 ==='Dolar'){
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05

    }else{
        const result = document.getElementById('result').innerHTML = "We can't change that currency"
    }

}

const currencyButton = document.getElementById('amount');

currencyButton.addEventListener('click', function change(){
    
    
})

move the declarations for money1 and money2 inside your function.

Out where they are right now, you get the value of the input immediately, not when the function is called.

(There are multiple problems and streamlines to be addressed here, but that’s your immediate “why doesnt this work” fix)

2 Likes

And to address your next-immediate one before you need to ask:

 if(money1 === 'Euro' && money2 === 'Dolar'){
        const result = document.getElementById('result').innerHTML = 'El cambio es' + money1 * 1.05

You cant multiply the string ‘Euro’ by 1.05 and expect to get a number.

1 Like

To add to that you need to change the above to just the handler function’s name — in this instance ‘change’

function change {
  if (money1....
}

currencyButton.addEventListener('click', change) // <- handler function to be called on click
1 Like

Yeah i saw that. I already changed the code and now it’s working thank you for the tips, whitout them i would still be lost.

I will let the code working here

const converted = document.getElementById('result');

function change(){
    const amount = document.getElementById('amount').value
    const money1 = document.getElementById('money1').value;
    const money2 = document.getElementById('money2').value
    const newMoney = amount
    if(money1 === 'Euro' && money2 === 'Dolar'){
        const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 1.03
        
    }else if(money1 === 'Euro' && money2 ==='Pound'){
        
        const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 0.87
        
    }else if(money1 === 'Dolar' && money2 ==='Euro'){
        
    const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 0.98
     
    
    }else if(money1 === 'Dolar' && money2 ==='Pound'){
        const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 0.89

    }else if(money1 === 'Pound' && money2 ==='Euro'){
        const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 1.15

    }else if(money1 === 'Pound' && money2 ==='Dolar'){
        const result = document.getElementById('result').innerHTML = 'El cambio es ' + newMoney * 1.17

    }else{
        const result = document.getElementById('result').innerHTML = "We can't change that currency"
    }

}

const currencyButton = document.getElementById('convert');

currencyButton.addEventListener('click', change)
    
    

That’s my cue! :wink:

Close those tags

The options in the datalist should be closed, or at least my code editor complains when they’re not closed.

    <datalist id="money">
      <option value="Euro"></option>
      <option value="Dolar"></option>
      <option value="Pound"></option>    
    </datalist>

Breaks are only for addresses and poetry

    </datalist>

    <br>

    <label>Amount:</label>

Instead of using a break element, a much more suitable solution is to use a paragraph element.

    <p>
        <label for="exampleDataList" class="form-label">Second Currency</label>
        ...
    </p>
    <p>
        <label>Amount:</label>
        ...
    </p>

JavaScript improvements

Because there are quite a lot of improvements to be made to the scripting code, it’s much easier to accept those ideas from an automated linting system than from another person.

Here are all of the things that the linter tells me need fixing:

  • use double quotes (instead of single quotes)
  • Expected ‘;’ and instead saw . . .
  • Line is longer than 80 characters.

Those are easily taken care of. The long lines were adjusted so that result was declared first, then result.innerHTML was used on the next lines.

    if (money1 === "Euro" && money2 === "Dolar") {
        const result = document.getElementById("result");
        result.innerHTML = "El cambio es" + money1 * 1.05;
    } else if (money1 === "Euro" && money2 === "Libra") {
        const result = document.getElementById("result");
        result.innerHTML = "El cambio es" + money1 * 1.05;
    } else if (money1 === "Dolar" && money2 === "Euro") {
        const result = document.getElementById("result");
        result.innerHTML = "El cambio es" + money1 * 1.05;

It makes the repetition more obvious, but that’s the whole point.

The result variable doesn’t need to be declared inside of the if statement, and can be moved outside of it instead.

    const result = document.getElementById("result");
    if (money1 === "Euro" && money2 === "Dolar") {
        result.innerHTML = "El cambio es" + money1 * 1.05;
    } else if (money1 === "Euro" && money2 === "Libra") {
        result.innerHTML = "El cambio es" + money1 * 1.05;
    } else if (money1 === "Dolar" && money2 === "Euro") {
        result.innerHTML = "El cambio es" + money1 * 1.05;

Improve the event listener

  • Redefinition of ‘change’ from line 6.
currencyButton.addEventListener("click", function change() {

});

The linter is right. Instead of defining a separate function called change, it’s better to have your own separate event handler there and from inside of the event handler, to call the change() function.

currencyButton.addEventListener("click", function currencyClickHandler() {
    change();
});

Getting values form the form fields

When running the code, the money, amount, and converted variables need to be defined inside of the change() function, so that only after you’ve filled in the form fields will it gain info from them.

function change() {
    const money1 = document.getElementById("money1").value;
    const money2 = document.getElementById("money2").value;
    const amount = document.getElementById("amount");
    const converted = document.getElementById("result");

Attaching the click event to the right element

Currently the event listener is attached to the input field for the amount.

const currencyButton = document.getElementById("amount");
currencyButton.addEventListener("click", function currencyClickHandler() {

That needs instead to be attached to the convert button.

// const currencyButton = document.getElementById("amount");
const currencyButton = document.getElementById("convert");
currencyButton.addEventListener("click", function currencyClickHandler() {

Use numbers, not text

When you get values from form fields, they are always retrieved as numbers.
It can help a lot to immediately convert them to numbers instead.

The money1 variable in your formula is the name “Euro”.

    if (money1 === "Euro" && money2 === "Dolar") {
        result.innerHTML = "El cambio es" + money1 * 1.05;

I suspect that you intend that to be the variable called amount instead.

    if (money1 === "Euro" && money2 === "Dolar") {
        // result.innerHTML = "El cambio es" + money1 * 1.05;
        result.innerHTML = "El cambio es" + amount * 1.05;

Get the amount as a number

Currently you are getting the amount element.

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

I suspect that you are instead wanting the numeric value that’s in that input field. That means using value to get the contents of the input field, and wrapping it with Number to turn it into a number.

    const amount = Number(document.getElementById("amount").value);

Conclusion

After resolving a number of different issues in the code, it is now in a better state from which further progress can be easily made.

function change() {
    const money1 = document.getElementById("money1").value;
    const money2 = document.getElementById("money2").value;
    const amount = Number(document.getElementById("amount").value);
    const converted = document.getElementById("result");

    const result = document.getElementById("result");
    console.log({money1: money1 === "Euro", money2: money2 === "Dolar"});
    if (money1 === "Euro" && money2 === "Dolar") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else if (money1 === "Euro" && money2 === "Libra") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else if (money1 === "Dolar" && money2 === "Euro") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else if (money1 === "Dolar" && money2 === "Libra") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else if (money1 === "Libra" && money2 === "Euro") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else if (money1 === "Libra" && money2 === "Dolar") {
        result.innerHTML = "El cambio es" + amount * 1.05;
    } else {
        result.innerHTML = "We can't change that currency";
    }

}

const currencyButton = document.getElementById("convert");
currencyButton.addEventListener("click", function currencyClickHandler() {
    change();
});

That updated code can be found at https://jsfiddle.net/ksa67dh0/

2 Likes

Having the output like that seems counter-productive - if you want to change the text, you’re now going to have to do it multiple times. Just try to calculate the conversion rate, then output it if you find a valid one.

Might be roughly the same amount of code, but maintenance should be easier later.

function change(){
    const amount = document.getElementById('amount').value
    const money1 = document.getElementById('money1').value;
    const money2 = document.getElementById('money2').value;
    let conversionRate = -1;
    if (money1 == 'Euro') {
        if (money2 == 'Dolar') {
            conversionRate = 1.03;
        } else if (money2 == 'Pound') {
            conversionRate = 0.87;
        }
    } else if (money1 == 'Dolar') {
        if (money2 == 'Euro') {
            conversionRate = 0.98;
        } else if (money2 == 'Pound') {
            conversionRate = 0.89;
        }
    } else if (money1 == 'Pound') {
        if (money2 == 'Dolar') {
            conversionRate = 1.17;
        } else if (money2 == 'Euro') {
            conversionRate = 1.15;
        }
    }
    document.getElementById('result').innerHTML = (conversionRate == -1) ? "We can't change that currency" : 'El cambio es ' + amount * conversionRate;
}
const currencyButton = document.getElementById('convert');
currencyButton.addEventListener('click', change)    
1 Like

Thanks. I followed you recomendations step by step, changing the quotes, closing the tags, and changing the else if, and i also added another else to give you a message when you try to change the same currency.

It’s really helpful to be able to follow your code and the steps you took to get there. Now, i think that is working as intended and it can be upgraded when i got more knowledge.

This is the code once all the changes are made.

<!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>Convertidor</title>
    <!-- CSS only -->
    <link href="https://cdn.jsdelivr.net/npm/bootstrap@5.2.1/dist/css/bootstrap.min.css" 
    rel="stylesheet" 
    integrity="sha384-iYQeCzEYFbKjA/T2uDLTpkwGzCiq6soy8tYaI1GyVh/UjpbCx/TYkiZhlZB6+fzT" crossorigin="anonymous">
    <link rel="stylesheet" href="moneda.css">
</head>
<body>
<form>
    <label for="exampleDataList" class="form-label">First currency</label>
    <input class="inputMoney" list="money" id="money1" placeholder="€,$,£,">
    <datalist id="money">
      <option value="Euro"></option>
      <option value="Dolar"></option>
      <option value="Pound"></option>
      
      
    </datalist>

    <label for="exampleDataList" class="form-label">Second Currency</label>
<input class="inputMoney" list="moneyConverted" id="money2" placeholder="€,$,£,">
<datalist id="moneyConverted">
     <option value="Euro"></option>
     <option value="Dolar"></option>
     <option value="Pound"></option>
     
</datalist>

<p>

<label>Amount:</label>
<input  type="number" id="amount" />


<button  type="button" id="convert">Convert</button>
</p>
<p id="result"></p>
</form>

     <!-- JavaScript Bundle with Popper -->
     <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.2.1/dist/js/bootstrap.bundle.min.js" integrity="sha384-u1OknCvxWvY5kfmNBILK2hRnQC3Pr17a+RTT6rIHI7NnikvbZlHgTPOOmMi466C8" crossorigin="anonymous"></script>
    <script src="moneda.js"></script>
</body>
</html>






function change(){
    const amount = document.getElementById("amount").value
    const money1 = document.getElementById("money1").value;
    const money2 = document.getElementById("money2").value
    const converted = document.getElementById("result");
    const newMoney = amount
    if(money1 === "Euro" && money2 === "Dolar"){
        result.innerHTML = "The change is " + newMoney * 1.03
        
    }else if(money1 === "Euro" && money2 ==="Pound"){
        
        result.innerHTML = "The change is " + newMoney * 0.87
        
    }else if(money1 === "Dolar" && money2 ==="Euro"){
        
        result.innerHTML = "The change is " + newMoney * 0.98
     
    
    }else if(money1 === "Dolar" && money2 ==="Pound"){
        result.innerHTML = "The change is " + newMoney  * 0.89

    }else if(money1 === "Pound" && money2 ==="Euro"){
        result.innerHTML = "The change is " + newMoney  * 1.15

    }else if(money1 === "Pound" && money2 ==="Dolar"){
        result.innerHTML = "The change is " + newMoney  * 1.17

    }else if(money1 === "Pound" && money2 ==="Pound"){
        result.innerHTML = "You are trying to change the same currency"

    }else if(money1 === "Dolar" && money2 ==="Dolar"){
        result.innerHTML = "You are trying to change the same currency"


    }else if(money1 === "Euro" && money2 ==="Euro"){
        result.innerHTML = "You are trying to change the same currency"

    }else{
        result.innerHTML = "We can't change that currency"
    }

}

const currencyButton = document.getElementById("convert");

currencyButton.addEventListener("click", function currencyClickHandler(){
    change();
})
    
    



Here is an alternative version of your function:

function change() {
  const amount = document.getElementById("amount").value;
  const money1 = document.getElementById("money1").value;
  const money2 = document.getElementById("money2").value;
  const converted = document.getElementById("result");

  if (money1 === money2) {
    result.innerHTML = "You are trying to change the same currency";
    return;
  }

  let newMoney = -1;
  if (money1 === "Euro" && money2 === "Dollar") {
    newMoney = amount * 1.03;
  } else if (money1 === "Euro" && money2 === "Pound") {
    newMoney = amount * 0.87;
  } else if (money1 === "Dollar" && money2 === "Euro") {
    newMoney = amount * 0.98;
  } else if (money1 === "Dollar" && money2 === "Pound") {
    newMoney = amount * 0.89;
  } else if (money1 === "Pound" && money2 === "Euro") {
    newMoney = amount * 1.15;
  } else if (money1 === "Pound" && money2 === "Dollar") {
    newMoney = amount * 1.17;
  }

  if (newMoney < 0) {
    result.innerHTML = "We can't change that currency";
    return;
  }

  result.innerHTML = "Second Currency Amount is " + newMoney.toFixed(2);
}

Note I have included toFixed(2) although 2 may not be appropriate for some currencies.

It would be good if you delete the second currency amount as soon as any input element is changed.

Consider using <select> instead of <datalist>. It’s your choice!

All your <label> elements need ‘for’ attributes each with value the same as the corresponding <input> element’s id.

Dollar is the name of more than 20 currencies.

1 Like

I’m a bit baffled by this. Why wrap the call to change in another function?

I could understand wrapping it if you wanted to pass some other arguments.

currencyButton.addEventListener("click", function currencyClickHandler(event) {
    change(event, someOtherArgument);
});

If I put the following through JSLint it passes

const myHandler = function () {
    console.log('clicked');
};

const button = document.querySelector('#button');
button.addEventListener('click', myHandler);

It’s late and I am bleary-eyed, so maybe I am missing something, but I don’t see the issue with.

currencyButton.addEventListener('click', change);
1 Like

The main reason is so that the job of the handler function is only to get information from the event object (if necessary) and pass that information on to other functions that use that info.

Even when no information is coming from the event object, it is a good standard structure to use that helps to provide a better understanding of what the different parts are for, and makes it easier for potential future improvements to be made as well.

Instead of having to restructure everything, you would only need to add an evt parameter to the handler, and you can then access any of the info that is needed from there.

I don’t like both ways.

As long as the change function is not used anywhere else I would directly insert the code into the addEventhandler.

If I want to understand code from another developer, I would first take a look what is done if the button is clicked. So I search for the part of the code, where the click handler for the button is added.
In your versions I will find it and get no new information. Instead I need to search for the change() function again.

Even if the change would be used one time somewhere else, it’s just 1 minute of work to extract it.

1 Like

I see where you are coming from and do agree with you. A handler that just handles makes sense.

What if you want to remove that eventListener though?

const button = document.querySelector('.click-me');
const disableBtn = document.querySelector('.disable-button');

const doSomething = function () { };

button.addEventListener('click', function buttonHandler () {
  doSomething();
});

disableBtn.addEventListener('click', function disableBtnHandler () {
  button.removeEventListener('click', buttonHandler); // Uncaught ReferenceError: buttonHandler is not defined
});

Wouldn’t this still be preferable?

const button = document.querySelector('.click-me');
const disableBtn = document.querySelector('.disable-click-me');

const doSomething = function () { };

const doSomethingElse = function () { };

const buttonHandler = function () {
  doSomething();
  doSomthingElse();
}

const removeButtonHandler = function () {
  button.removeEventListener('click', buttonHandler);
}

button.addEventListener('click', buttonHandler);
disableBtn.addEventListener('click', removeButtonHandler);

Currently the buttonHandler is an inline function expression, so it can’t be seen by other parts of the code.

Extract that buttonHandler function to be a standard function definition, and you can then use the following:

function buttonHandler() {
  doSomething();
}

button.addEventListener('click', buttonHandler);

disableBtn.addEventListener('click', function disableBtnHandler () {
  button.removeEventListener('click', buttonHandler);
});

I agree, extracting handler functions is a good next step to take, and can be done before it’s required by the code, or on an as-needed basis.

1 Like

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