3
\$\begingroup\$

Please can any code experts review and improve my code?

HTML

<!DOCTYPE html>
<html>
  <head>
    <title>API</title>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
    <link rel="stylesheet" href="//fonts.googleapis.com/css?family=font1|font2|etc" type="text/css">
    <link rel="stylesheet" type="text/css" href="style.css">
  </head>
  <body>
      <h1 id="test"></h1>
      <div class="weather-container">
        <img id="icon">
        <h1 id="weather"></h1>
        <p>Description: <span id="desc"></span></p>
        <p>Temperature: <span id="temp"></span></p>
      </div>
      <script src="mousetrap.js"></script>
      <script src="moment.js"></script>
      <script src="jquery.js"></script>
      <script src="script.js"></script>
  </body>
</html>

That's the HTML. It is just mostly tags with id to output info.

JavaScript

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.");
$.getJSON("https://api.openweathermap.org/data/2.5/weather?q="+ask+"&units=metric&appid=32b8cd17f2ff5d84d72342dd7408bab2", function(data) {
    console.log(data);
    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 + "℃"
    $("#icon").attr("src", icon);
    document.getElementById('weather').innerHTML = weather;
    document.getElementById('desc').innerHTML = desc;
    document.getElementById('temp').innerHTML = temp1;
});

If there is any other language than jQquery that I can use to get JSON files from API, please let me know because I find jQuery quite confusing.

Improve this question
\$\endgroup\$
2
  • \$\begingroup\$ Hello welcome to CR. This looks interesting. If there is any other language than Jquery -> well jQuery is not a language it is a library. There are various ways to call APIs see here \$\endgroup\$ – bhathiya-perera Feb 23 '19 at 17:20
  • \$\begingroup\$ Welcome to Code Review! You can get a line break without a blank line: just append two blanks at the end of the line that shall be followed by the break. \$\endgroup\$ – greybeard Feb 23 '19 at 23:08
1
\$\begingroup\$

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 + "℃"
$("#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 + "℃";

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 => {
    ...
});
Improve this answer
\$\endgroup\$
1
\$\begingroup\$

Response to your question

If there is any other language than Jquery that I can use to get json files from api, please let me know because i find Jquery quite confusing :-).

As was mentioned in a comment (as well as a link-only answer that has since been deleted), there are alternatives, including but not limited to:

General review

User experience

You didn't state whether the specification was part of an assignment/task or just something you came up with, but the UX might be improved by having the prompt and Ajax request triggered by an event like a mouse click instead of page load.

If such a change was implemented, then it would be wise to cache the DOM references in variables instead of querying the DOM each time- e.g. 

var weatherEl = document.getElementById('weather');
var descEl = document.getElementById('desc');
var tempEl = document.getElementById('temp');

//function to prompt

//AJAX response callback
function ajaxResponse(data) {
    //parse data
    //...
    weatherEl.innerHTML = data.weather[0].description;
    descEl = data.weather[0].description;
    tempEl.innerHTML = data.main.temp + "°C";
}

That way, the lines can be shorter, the DOM won't get queried each time, and you can likely eliminate those variables that are assigned and then only used once.

Perhaps a template system would help improve the process of updating the DOM elements instead of having to update each item individually.

Handling other responses and errors

The code above doesn't appear to (gracefully) handle any response other than a successful call to the API endpoint. When the user types in a city that the API doesn't recognize, then a 404 response is returned. If you continue to use the jQuery library, then a .fail() callback could be specified (see the $.getJSON() documentation for an example to handle that. Also, if a server error occurred, then a 5xx response might be returned.

Additionally, the code assumes that data.weather will be an array and have at least 1 element. What happens if either of those are not true? While it may seem implausible it may be possible and something to guard against.

Extra libraries

The code includes mousetrap.js and moment.js - I presume those correspond to the libraries Mousetrap and MomentJS, but it doesn't appear that those libraries are used by the code. Unless those are used by other code not included, those libraries can be removed to save the users time loading the page.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.