How is my Python code? (raw.githubusercontent.com)

submitted 7 months ago* (last edited 7 months ago) by the_citizen@lemmy.world to c/python@programming.dev

20 comments fedilink hide all child comments

I don't know if it's the true place to ask, apologizing if not. I started to python one and half week ago. So I'm still beginner.

I made a terminal based weather application with python. What do you think about the code, is it good enough? I mean is it professional enough and how can I make the same functions with more less code?

Here's the main file (I also added it as url to post): https://raw.githubusercontent.com/TheCitizenOne/openweather/refs/heads/main/openweather.py
Here's the config.json file: https://raw.githubusercontent.com/TheCitizenOne/openweather/refs/heads/main/config.json

you are viewing a single comment's thread
view the rest of the comments

[-] cypherpunks@lemmy.ml 7 points 7 months ago

I started to python one and half week ago. So I’m still beginner.

Nice work! Here are a few notes:

The WeatherApp object has a mix of attributes with long-term (eg self.LOCATIONS) and short-term (eg self.city) relevance. Instance attributes introduced in places other than __init__, which makes it non-trivial for a reader to quickly understand what the object contains. And, actually, self.{city,lat,lon} are all only used from the add_city method so they could/should be local variables instead of instance attributes (just remove the self. from them).

There seem to maybe be some bugs around when things are lowercase and when not; for example checking if self.city.lower() in self.LOCATIONS but then when writing there the non-lower self.ctiy is used as the key to self.LOCATIONS.

The code under if rep == "1" and elif rep == "2" is mostly duplicated, and there is no else branch to cover if rep is something other than 1 or 2.

It looks like the config only persists favorites so far (and not non-favorite cities which the user can add) which isn't obvious from the user interface.

Passing both location and locations into WeatherAPI so that it can look up locations[location] is unnecessary; it would be clearer to pass in the dict for the specific location. It would also be possible to avoid the need for LOWLOCATIONS by adding a non-lowercase name key to the per-location dictionaries that just have lat and lon right now, and then keeping LOCATIONS keyed by the lowercase names.

HTH! happy hacking :)