Skip to main content
Code Review

Return to Answer

fix indentation
Source Link

Variables

var icon = "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";
var weather = data.weather[0].main;
var desc = data.weather[0].description;
var temp = data.main.temp;
var temp1 = temp + "°C"
$("#icon").attr("src", icon);
document.getElementById('weather').innerHTML = weather;
document.getElementById('desc').innerHTML = desc;
document.getElementById('temp').innerHTML = temp1;

None of these variables are really needed. Each variable is only used once and you aren't doing any incredibly complex calculations on them, so they aren't even needed for the sake of understanding. Eliminating variables would also get rid of this temp1 variable which is frankly a little ugly -- usually you know you've gone too far when you have to start adding numbers to variable names.

Try something like this instead:

$("#icon").attr("src", "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";);
document.getElementById('weather').innerHTML = data.weather[0].main;
document.getElementById('desc').innerHTML = data.weather[0].description;
document.getElementById('temp').innerHTML = data.main.temp + "°C";

No variables, same functionality, just as understandable.

You don't even need the ask variable, but I can understand why you're using it because it is indeed a long question.

jQuery

jQuery is not very relevant in today's JavaScript. I would recommend learning about fetch and thus Promises. This design is much cleaner and much nicer to work with, IMO.

Your code with fetch would look like this:

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");
fetch(*url*).then(r => r.json()).then(data => {
 ...
});

Variables

var icon = "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";
var weather = data.weather[0].main;
var desc = data.weather[0].description;
var temp = data.main.temp;
var temp1 = temp + "°C"
$("#icon").attr("src", icon);
document.getElementById('weather').innerHTML = weather;
document.getElementById('desc').innerHTML = desc;
document.getElementById('temp').innerHTML = temp1;

None of these variables are really needed. Each variable is only used once and you aren't doing any incredibly complex calculations on them, so they aren't even needed for the sake of understanding. Eliminating variables would also get rid of this temp1 variable which is frankly a little ugly -- usually you know you've gone too far when you have to start adding numbers to variable names.

Try something like this instead:

$("#icon").attr("src", "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";);
document.getElementById('weather').innerHTML = data.weather[0].main;
document.getElementById('desc').innerHTML = data.weather[0].description;
document.getElementById('temp').innerHTML = data.main.temp + "°C";

No variables, same functionality, just as understandable.

You don't even need the ask variable, but I can understand why you're using it because it is indeed a long question.

jQuery

jQuery is not very relevant in today's JavaScript. I would recommend learning about fetch and thus Promises. This design is much cleaner and much nicer to work with, IMO.

Your code with fetch would look like this:

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");

fetch(*url*).then(r => r.json()).then(data => {
 ...
});

Variables

var icon = "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";
var weather = data.weather[0].main;
var desc = data.weather[0].description;
var temp = data.main.temp;
var temp1 = temp + "°C"
$("#icon").attr("src", icon);
document.getElementById('weather').innerHTML = weather;
document.getElementById('desc').innerHTML = desc;
document.getElementById('temp').innerHTML = temp1;

None of these variables are really needed. Each variable is only used once and you aren't doing any incredibly complex calculations on them, so they aren't even needed for the sake of understanding. Eliminating variables would also get rid of this temp1 variable which is frankly a little ugly -- usually you know you've gone too far when you have to start adding numbers to variable names.

Try something like this instead:

$("#icon").attr("src", "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";);
document.getElementById('weather').innerHTML = data.weather[0].main;
document.getElementById('desc').innerHTML = data.weather[0].description;
document.getElementById('temp').innerHTML = data.main.temp + "°C";

No variables, same functionality, just as understandable.

You don't even need the ask variable, but I can understand why you're using it because it is indeed a long question.

jQuery

jQuery is not very relevant in today's JavaScript. I would recommend learning about fetch and thus Promises. This design is much cleaner and much nicer to work with, IMO.

Your code with fetch would look like this:

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");
fetch(*url*).then(r => r.json()).then(data => {
 ...
});
Source Link
SirPython
  • 13.4k
  • 3
  • 38
  • 93

Variables

var icon = "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";
var weather = data.weather[0].main;
var desc = data.weather[0].description;
var temp = data.main.temp;
var temp1 = temp + "°C"
$("#icon").attr("src", icon);
document.getElementById('weather').innerHTML = weather;
document.getElementById('desc').innerHTML = desc;
document.getElementById('temp').innerHTML = temp1;

None of these variables are really needed. Each variable is only used once and you aren't doing any incredibly complex calculations on them, so they aren't even needed for the sake of understanding. Eliminating variables would also get rid of this temp1 variable which is frankly a little ugly -- usually you know you've gone too far when you have to start adding numbers to variable names.

Try something like this instead:

$("#icon").attr("src", "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";);
document.getElementById('weather').innerHTML = data.weather[0].main;
document.getElementById('desc').innerHTML = data.weather[0].description;
document.getElementById('temp').innerHTML = data.main.temp + "°C";

No variables, same functionality, just as understandable.

You don't even need the ask variable, but I can understand why you're using it because it is indeed a long question.

jQuery

jQuery is not very relevant in today's JavaScript. I would recommend learning about fetch and thus Promises. This design is much cleaner and much nicer to work with, IMO.

Your code with fetch would look like this:

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");

fetch(*url*).then(r => r.json()).then(data => {
 ...
});
lang-html

AltStyle によって変換されたページ (->オリジナル) /