Weather App- Python

Question

This is my very basic Weather App. Would appreciate your opinion mainly about style, writing code etc. I know there is not much functionality to this app, but I have created it just to practice and to share with you guys so you could possibly point weak points I need to focus on as a programmer. As I am learning mainly from materials I find online, the opinion of professionals will be much appreciated so I can go in the right direction.

enter image description here

import tkinter as tk
import requests, json, os
from tkinter import StringVar, ttk
from pathlib import Path
class Window(tk.Tk):
 def __init__(self, database: object, icons: str) -> None:
 super().__init__()
 self._db = database
 self._icons = icons
 
 self.enter_city()
 self.update_vars()
 self.labels()
 self.weather_img()
 # auto update function, 'while typing' style 
 self.update_data()
 
 self.geometry('350x450')
 self.title('Weather App')
 self.iconbitmap(f'{icons}\\icon.ico')
 def weather_img(self) -> None:
 # set weather image
 self.icon = StringVar()
 self.icon.set(WeatherData.get_icon_code(self.weather))
 # weather icon path
 weather_icon = Path(f'{icons}\\{self.icon.get()}.png')
 # ensure during changing city when icon code not availabe there is no error, refresh icon on the screen
 if weather_icon.is_file():
 self.weather_image = tk.PhotoImage(file=f'{icons}\\{self.icon.get()}.png')
 else:
 self.weather_image = tk.PhotoImage(file=f'{icons}\\refresh.png')
 self.image_lbl = ttk.Label(self, image=self.weather_image)
 self.image_lbl.place(x=45, y=35) 
 def enter_city(self) -> None:
 # City entry
 self.city_ent = ttk.Entry(self, justify='center', width=50)
 self.city_ent.place(x=20, y=10)
 self.city_var = StringVar(value='Warsaw')
 def update_vars(self) -> None:
 # Get weather data for City
 self.weather = WeatherData.get_weather(self._db, self.city_var.get())
 
 # set vars for auto update
 self.temp = StringVar()
 self.temp.set(WeatherData.get_temp(self.weather))
 self.wind = StringVar()
 self.wind.set(WeatherData.get_wind_direction(self.weather))
 self.prediction = StringVar()
 self.prediction.set(WeatherData.get_predicted_weather(self.weather))
 def labels(self) -> None:
 self.city_lbl = ttk.Label(self, textvariable=self.city_var, width=35, font=('Helvetica', 12, 'bold'), anchor='center')
 self.city_lbl.place(x=10, y=290)
 self.city_ent.configure(textvariable=self.city_var)
 self.temp_lbl = ttk.Label(self, text='Temperature:', font=('Helvetica', 11))
 self.temp_lbl.place(x=10, y=330)
 self.wind_lbl = ttk.Label(self, text='Wind direction:', font=('Helvetica', 11))
 self.wind_lbl.place(x=10, y=360)
 self.prediction_lbl = ttk.Label(self, text='Predicted weather:', font=('Helvetica', 11))
 self.prediction_lbl.place(x=10, y=390)
 self.temp_val_lbl = ttk.Label(self, textvariable=self.temp, font=('Helvetica', 11))
 self.temp_val_lbl.place(x=220, y=330)
 
 self.wind_val_lbl = ttk.Label(self, textvariable=self.wind, font=('Helvetica', 11))
 self.wind_val_lbl.place(x=220, y=360)
 self.prediction_val_lbl = ttk.Label(self, textvariable=self.prediction, font=('Helvetica', 11))
 self.prediction_val_lbl.place(x=220, y=390)
 
 
 def update_data(self) -> None:
 self.weather = WeatherData.get_weather(self._db, self.city_var.get())
 self.temp.set(WeatherData.get_temp(self.weather))
 self.wind.set(WeatherData.get_wind_direction(self.weather))
 self.prediction.set(WeatherData.get_predicted_weather(self.weather))
 # destroy icon label, so new one won't be on top of old one
 self.image_lbl.destroy()
 # refresh icon label
 self.weather_img()
 self.after(1000, self.update_data)
class WeatherData:
 def __init__(self, key: str) -> None:
 self._key = key
 # excepting KeyError for autoupdate. App won't stop working while typing new city name
 def get_weather(self, city: str) -> list:
 url = f'https://api.openweathermap.org/data/2.5/weather?q={city}&units=metric&appid={self._key}'
 response = requests.get(url)
 data = response.json()
 return data
 def get_temp(data: list) -> str:
 try:
 celsius = data['main']['temp']
 return str(round(celsius))
 except KeyError:
 pass
 def get_predicted_weather(data: list) -> str:
 try: 
 sky = data['weather']
 for item in sky:
 for key, value in item.items():
 if key == 'main':
 return str(value)
 except KeyError:
 pass
 def get_wind_direction(data: list) -> str:
 try: 
 wind = data['wind']['deg']
 if wind == 0:
 return 'North'
 elif wind > 0 and wind < 90:
 return 'North East'
 elif wind == 90:
 return 'East'
 elif wind > 90 and wind < 180:
 return 'South East'
 elif wind == 180:
 return 'South'
 elif wind > 180 and wind < 270:
 return 'South West'
 elif wind == 270:
 return 'West'
 else:
 return 'North West'
 except KeyError:
 pass
 # get icon code to display right weather icon
 def get_icon_code(data: list) -> str:
 try: 
 info = data['weather']
 for item in info:
 for key, value in item.items():
 if key == 'icon':
 return value
 except KeyError:
 pass
if __name__ == "__main__":
 _current_path = os.path.dirname(__file__)
 
 # Free API key
 with open(f'{_current_path}\\settings\\apiconfig.json', 'r') as f:
 api_file = json.load(f)
 for key, value in api_file.items():
 if key == 'key':
 _api_key = value
 
 # icons folder
 icons = f'{_current_path}\\icons'
 
 db = WeatherData(_api_key)
 Window(db, icons).mainloop()

Answer 1

• Don't hint database as an object; hint it as a WeatherData. To resolve the missing symbol, move Window below WeatherData. If for whatever reason you weren't able to do that, you could also hint it in string syntax, i.e. 'WeatherData'.
• Avoid inheriting from tk.Tk and instead put it in a member variable ("has-a", not "is-a"). I say this because you don't override any of the parent functions and you don't reference any of its member variables from what I see.
• Rather than hard-coding backslashes in your paths, make icons a pathlib.Path, and use the / operator to get to icon.ico
• self.icon isn't referenced outside of weather_img so it doesn't need to be a member variable; similar for the member variables weather_image, city_lbl, etc.
• Repeated expressions like this:

 ttk.Label(self, text='Temperature:', font=('Helvetica', 11))

can be reduced by

label = partial(ttk.Label, self, font=('Helvetica', 11))
self.temp_lbl = label(text='Temperature:')
# etc.

• It's a good idea to represent WeatherData as a class, but not such a good idea to have most of its member functions accept a data parameter. If you move get_weather outside of the class and pass its result to WeatherData's constructor. Save it to a member variable. Modify get_temp to have a self parameter, no data parameter, and use its member variable; similar with other functions. This way, WeatherData is actually meaningful as a class, and is effectively immutable.
• Do not format your query parameters into your URL. Pass them in the params dict argument to get().
• Use the return value of get, the response, in a with.
• Call response.raise_for_status().
• Since you have exception handling around data['main']['temp'], the return type can't be str, but instead Optional[str]. Similar for other functions.
• It's good that you have a main guard, but it's not enough. All of those variables are still in global scope. Make a main function and call it from your main guard.

• Jakub: Thank you very much. Another few things learnt. As to Optional[str] always thought if there is exception, that doesn't count as it's excepted, so it would be only str, learning something everyday. Thank you for your time to review it, means and helps a lot.
• Reinderien: It's not Optional because there's an exception - it's Optional because you catch the exception and then implicitly return None. If you were to let the exception fall through, then the hint would remain str.
• Jakub: I got it now. Thank you for explaining.
• Schmuddi: I have two questions about the suggestion to reduce repeated expressions by partial(): (1) Just to clarify, it's partial() from the functools module, right? (2) Is this replacement commonly considered to be good style? I've never used partial() before, so I'm not experienced with its use cases. But at first glance, it somewhat obscures that label() creates GUI elements, especially if you miss the place where it has been defined. I'd have guessed that the explicit ttk.Label() was to be preferred here. So, perhaps you could elaborate a bit why partial() is the better choice?
• Reinderien: @Schmuddi 1. correct; 2. It isn't strictly a better choice, but it eliminates a lot of repeated code. Another alternative is to make a dedicated class method that does an equivalent label construction. A better name such as make_label could be used to address your concerns.

Answer 2

• Destroying and re-creating the entire icon label feels like an awkward solution. Isn't it possible to keep the label as it is and only replace its image? I believe something like self.image_lbl.configure(image=self.weather_image) might do that

  • I'm also wondering if we actually need to do that manually. We might be able to use self.icon.trace_add to automatically update the icon when that variable changes. This might look a bit like so:

def enter_city(self) -> None:
 self.icon = StringVar()
 def icon_change_callback(_: str, _name: str, _mode: str):
 icon_path = Path(self._icons, f"{self.icon.get()}.png")
 if icon_path.is_file():
 self.weather_image = tk.PhotoImage(file=icon_path)
 else:
 self.weather_image = tk.PhotoImage(file=Path(self._icons, "refresh.png"))
 self.image_lbl.configure(image=self.weather_image)
 self.icon.trace_add("write", icon_change_callback)
 self.image_lbl = ttk.Label(self)
 self.image_lbl.place(x=45, y=35)

• That still ends up re-loading the entire icon file every second, though. That's not ideal. It'd probably be better to keep the PhotoImages around somewhere so we can look them up without having to bother wish disk I/O

• We might be able to use callbacks with self.city_var as well - that might let us update right away when the variable gets updated, while not having to update every second the rest of the time. The weather doesn't usually change that fast after all

• Why is WeatherData a class anyway? Most of the methods don't interact with the class at all, so they could just as easily be standalone functions. The only exception is get_weather, but if we only have one method, we usually aren't any better off by using a class than we'd be if we'd just used a function

• Those loops in get_icon_code and get_predicted_weather feel awkward. It seems to me like they can be simplified to data['weather'][0]['main'] - am I missing something?

  • If we can do that, most of those helper functions turn into one-liners that just get data from a known location in a dict. As much as I like functions, I'm not sure we actually need to keep those

• get_wind_direction feels like it has two jobs - getting a wind angle from an API result and turning an angle into a compass direction. Those aren't that closely related, and kind of feel like they could belong in two different functions

• Similarly, update_vars is responsible for creating variables, while update_data is clearly responsible for updating the program's state. So I'd say we could get rid of the updating part of update_vars, maybe rename it to create_vars or something

• Speaking of names, all of Window's methods exist to perform actions, and should probably be named after the actions they perform. labels and weather_img feel less descriptive than create_labels and create_weather_image. And while enter_city and update_vars do describe actions, those are not the actions those functions perform - enter_city creates an entry field, and as mentioned update_vars doesn't update existing variables but instead creates new ones

• The data parameters of WeatherData's methods all seem to have the wrong type - the type hints say list, but they're dicts

  • Those functions also claim to always return str, but they may return None. Either allow them to raise those exceptions instead of swallowing them, change the return type to typing.Optional[str], or make them return some kind of str even if the lookup fails

• weather_img depends on the global variable icons, which I don't think we want. We already save that variable as self._icons during __init__, so I see no reason not to use that instead

• Side note - it feels a bit weird to me that get_wind_direction only returns "north", "south", "east" and "west" if the angle is exactly 0, 180, 90 or 270 degrees - I'd say 89 degrees is more east than north-east

• While building a query string by hand works fine, it's often neater to use the params argument of request.get's params options:

requests.get('https://api.openweathermap.org/data/2.5/weather', params={
 "q": city,
 "units": "metric",
 "appid": key
})

• Chained comparisons like 90 < angle and angle < 180 can be simplified to 90 < angle < 180

• Different operating systems have different ways of separating the parts of a file path. For portability, it might be better to not provide the path as a single string, but instead use Path to combine the pieces like Path(_current_path, 'icons') instead of f"{_current_path}\\icons"

• Jakub: Thank you for your answer. A lot to learn. As to the image, never did anything like that yet and that's the only way it actually work, but now I am going to try to do it as you said. I kept class WeatherData as I am aiming to make this app a bit more useful, so I will add some extra features like next day weather prediction etc etc, but want to do it right way. Definitely will use your advice to use Path rather than path as string. Wind direction will be changed, more or less if wind is closer to value return south or west etc (89 will be closer to east ) just didn't work out how to do it yet.