Code Review Asked by Mohammad Abu Sahadat on January 7, 2021
I am learning JavaScript. Please review my code. Just created this Celsius & Fahrenheit Converter. Any suggestions to improve my code will be really helpful.
const celsiusToFahrenheit = document.getElementById('celsiusToFarhenhite');
const fahrenheitToCelsius = document.getElementById('farhenhiteToCelsius');
const converterButton = document.querySelector('.calculate');
converterButton.addEventListener('click', function(event){
event.preventDefault();
const celsiusToFahrenheitValue = (celsiusToFahrenheit.value * 9/5 + 32);
const fahrenheitToCelsiusValue = (fahrenheitToCelsius.value - 32) * 5/9;
if(celsiusToFahrenheit.value){
document.write(`<h1>Celcius To Fahrenheit Result: </h1> ${celsiusToFahrenheitValue} ℉`);
}else if (fahrenheitToCelsius.value){
document.write(`<h1>Fahrenheit To Celcius Result: </h1> ${fahrenheitToCelsiusValue} ℃`);
}else{
document.write(`<h1>You must enter a number!</h1>`);
}
})
Use proper spelling - it'll looks more professional and can prevent bugs stemming from the typo. Farhenhite should be Fahrenheit, and Celcius should be Celsius.
Use proper indentation - every new block should open a new indentation level. You're doing this well with the if/elses, but you should also do it for the whole click listener as well:
converterButton.addEventListener('click', function(event){
event.preventDefault();
const celsiusToFahrenheitValue = (celsiusToFahrenheit.value * 9/5 + 32);
const fahrenheitToCelsiusValue = (fahrenheitToCelsius.value - 32) * 5/9;
// etc
preventDefault? Is the button inside a <form>? If so, the preventDefault is fine. Otherwise, it won't do anything and can be removed.
Avoid document.write - it can be insecure and somewhat confusing. Here, since it's called after the page loads, it will replace the whole document with the new HTML, which is probably not what you want - you'd probably rather preserve the converter and let people continue to convert after pressing the button once. Instead, populate, say, a results element with the results.
Confusing variable names Without reading the variable definition, it isn't entirely clear what the difference between celsiusToFahrenheitValue and celsiusToFahrenheit.value is. Consider only calculating the new value when it's needed, inside the if statements, and calling it something like convertedToFah. Also consider renaming celsiusToFahrenheit to just plain celsius, since the value it will hold is the plain celcius value.
<h1 class='conversion-type'></h1>
<div class='result'></div>
if (celsius.value) {
const convertedToFah = (celsius.value * 9/5 + 32);
document.querySelector('.conversion-type').textContent = 'Celsius To Fahrenheit Result:';
document.querySelector('.result').textContent = `${convertedToFah} ℉`;
}
When one input is changed, consider clearing the other input to make it clear what will be converted when the button is pressed - use an input listener.
Correct answer by CertainPerformance on January 7, 2021
I think you should write your code to more clearly treat nouns as nouns and verbs as verbs. celsiusToFahrenheit is a noun, but its name makes it sound like a verb. Getting rid of the to part makes your variable names simpler, and more clearly nouns.
The line celsiusToFahrenheitValue = (celsiusToFahrenheit.value * 9/5 + 32); is a verb, but you're doing it in a const declaration, making it seem like a noun. In addition, you only need to calculate one. Moving the declaration into if-branches both saves calculation, makes it clearer that you're doing a calculation, and allows you to not introduce another const.
Your if-then logic catches the case where neither are set, but doesn't anticipate the possibility that both are set. You should either disable one entry when the other is entered, or have logic dealing with this case. You could also separate out the two converter: have an input box for Celcius, a "convert from Celcius to Farenheit" button, and an output box for the calculation, and then another input box for Farenheit, "convert from Farenheit to Celcius" button, and another output box for that output.
Some of your lines are almost 100 characters long. If a language uses semicolons to demarcate commands, that implies that it ignores carriage returns. So if a line is getting long, you can just put a line break in.
Javascript isn't my primary language, so hopefully I have the syntax correct on this:
const Celcius = document.getElementById('celciusToFarhenheit');
const Fahrenheit = document.getElementById('farhenheitToCelcius');
const converterButton = document.querySelector('.calculate');
converterButton.addEventListener('click',
function(event){
event.preventDefault();
if(Celcius.value && !Farenheit.value){
document.write(`<h1>Celcius To Fahrenheit Result: </h1>
${Celcius.value * 9/5 + 32 } ℉`);
}
if(Farenheit.value && !Celcius.value){
document.write(`<h1>Celcius To Fahrenheit Result: </h1>
${(Fahrenheit.value - 32) * 5/9} ℃`);
}
if (Celcius.value && Farenheit.value){
document.write(`<h1>Enter only one number.</h1>`);
}
if !(Celcius.value || Farenheit.value){
document.write(`<h1>You must enter a number!</h1>`);
}
}
)
Answered by Acccumulation on January 7, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP