Weather application data structures and design

Question

Relevant files are script.ts, WeatherData.ts, index.html.

I made a simple weather 5-day forecast application using mainly TypeScript. I was wondering how I can improve my code since I originally wanted to use a data structure that I created called a "WeatherData" to store my information about the weather for each day of the week.

GitHub project

WeatherData data structure

export class WeatherData {
    date: string;
    city: string;
    country: string;
    temperature: number;
    minTemperature: number;
    maxTemperature: number;
    weather: any;
    weatherIcon: any;
}

TypeScript file concerns

The main concerns I have about the TypeScript files (script.ts, WeatherData.ts) is the fact that halfway through making my project, I realized that I didn't actually need to use a WeatherData data structure. This led to me passing in WeatherData variables in functions as well as returning WeatherData variables from functions that don't actually need to return it.

1. Is there a more object-oriented way I should be approaching my script.ts class in order to use data structures?
2. Will using a WeatherData data structure be better?
3. How can I format my code (maybe different classes?) such that all the functions aren't all clumped together?

HTML file concerns

First time ever making a front-end UI using HTML and I don't know what areas I should refactor. I noticed I have a lot of repeated element tags such as:

<h4 id="wordDay0">Day 0</h4>
<h4 id="monthDay0">month-day 0</h4>
<h4 id="forecast0">Forecast</h4>
<img src="" id="icon0" alt="" height="60" width="60">
<h1 id="currentTemp0">Current</h1>
<h4 id="maxTemp0">Max</h4>
<h4 id="minTemp0">Min</h4> 

I have 5 of these (one for each day).

Screenshot of what the UI looks like:

1. What type of suggestions are there to making my UI look better? I was thinking of making the jumbotron element a different color.

Answer 1

unor already covered pretty much everything I would say for the HTML structure, so I will focus on script.ts.

1. The most important advice I can give you is to get rid of any. By using any you are completely negating the advantages of using Typescript. Properly type everything possible. In this case, it looks like you are using any mostly for the response from the API. It would be far better to use an interface for the API response. It could look something like this.

2. Avoid explicitly stating types. There is a place and time for stating what type a variable is, but it is often just more noise to read.

   const daysShort: string[] = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'];
   

   There is no need to state that daysShort is an array of strings as the compiler is smart enough to determine that itself. Personally, I try to only define types when using objects. Type inference for primitive types and arrays is generally very good.

3. Don't mix const/let and var. Be consistent. I recommend sticking with const/let.

4. Don't use functions that return promises as event handlers. This doesn't make sense as the event handler won't know what to do with them.

5. If using $.ajax don't append query parameters yourself, use the data property of the settings object. (next point)

6. customSearch doesn't make sense. Instantiating a promise will never throw an error. If you want to handle errors from the promise, you need to use .catch(error => {}) or use async/await. It would be better to write the function like this:

   async function customSearch() {
     const searchParameter: string = (<HTMLInputElement>document.getElementById("searchBox")).value;
     if (searchParameter === "") {
       alert("Please enter a city and it's ISO country code.")
     }
   
     try {
       const response: ApiResponse = await $.getJSON("https://api.openweathermap.org/data/2.5/forecast", {
         data: {
           q: searchParameter,
           appid: appid
         }
       })
       if (response.cod !== "200") {
         throw new Error(response.message)
       }
   
       setGeneralInforation(response)
     } catch (error) {
       showError("API returned an error, invalid city / country code?");
     }
   }
   

   (I would make similar comments for your other search functions, and I would also personally split out the error handling from the API request so that the function would end up looking more like the following)

   async function customSearch(searchParameter: string): Promise<ApiOkResponse > {
     if (searchParameter === "") {
       throw new Error("Please enter a city and it's ISO country code.");
     }
   
     const response: ApiResponse = $.getJSON("https://api.openweathermap.org/data/2.5/forecast", {
       data: {
         q: searchParameter,
         appid: appid
       }
     })
     if (response.cod !== "200") {
       throw new Error("Invalid request: " + response.message)
     }
   
     return response;
   }
   

7. I suspect that determineMinTemp, determineMaxTemp, determineCurrentTemp, (or at least min/max) can be mostly combined by using a function which accepts a function as a parameter to get the correct key, but this is difficult to say without better typings, feel free to ask another question once you have incorporated suggestions from this round of reviews to see what other suggestions there may be.

There is certainly more that could be said, but I think for now I'll leave you with this so this doesn't become too overwhelming. Your project isn't a bad start!

---

A few opinionated statements that some may not agree with.

1. You probably don't need jQuery here. You essentially use it for document.querySelector, .addEventListener('click', fn), and fetch. I would probably remove it.

2. Add tslint to the project and configure it to warn about consistent spacing (if( vs if (), this can also help with autofixing missing semicolons, etc.

3. Add noUnusedLocals and noUnusedParameters to your tsconfig.json file to avoid importing code you don't need. You import parse from ts-node/dist, but don't use it.

Answer 2

About the HTML

You shouldn’t use heading elements (h1-h6) like that. These elements give the heading for the section they create. Don’t use them for the name in simple name-value items, nor for stand-alone content (where no other content in the section exists).

You could represent each day as a section:

<section><!-- day 1 --></section>
<section><!-- day 2 --></section>
<section><!-- day 3 --></section>
<section><!-- day 4 --></section>
<section><!-- day 5 --></section>

A section should ideally have a heading (and it’s the only one needed in this case). I think it makes the most sense to use the day+date as heading. The time element can be used to mark up the date in a machine-readable way. The abbr element can be used to give the full name of the day.

<h2><abbr title="Sunday">Sun</abbr> <time datetime="2017-12-31">Dec. 31</time></h2>

The forecast can be given in a p. As the img seems to convey the same content, it’s correct to keep the alt attribute empty.

<p>clear sky</p>
<img src="clear-sky.png" alt="" height="60" width="60">

While you could use a list for the temperatures, I think three p elements are sufficient in this simple case.

<p>14 °C</p>
<p>High: 20 °C</p>
<p>Low: 6 °C</p>

When using a label for the first temperature, too, I would go with a dl element instead:

<dl>
  <dt>Current</dt> <dd>14 °C</dd>
  <dt>High</dt> <dd>20 °C</dd>
  <dt>Low</dt> <dd>6 °C</dd>
</dl>

---

So, an entry for a day could look like this:

<section>
  <h2><abbr title="Sunday">Sun</abbr> <time datetime="2017-12-31">Dec. 31</time></h2>
  <p>clear sky</p>
  <img src="clear-sky.png" alt="" height="60" width="60">
  <p>14 °C</p>
  <p>High: 20 °C</p>
  <p>Low: 6 °C</p>
</section>

or with dl:

<section>
  <h2><abbr title="Sunday">Sun</abbr> <time datetime="2017-12-31">Dec. 31</time></h2>
  <p>clear sky</p>
  <img src="clear-sky.png" alt="" height="60" width="60">
  <dl>
    <dt>Current</dt> <dd>14 °C</dd>
    <dt>High</dt> <dd>20 °C</dd>
    <dt>Low</dt> <dd>6 °C</dd>
  </dl>
</section>