this post was submitted on 25 Apr 2025
23 points (89.7% liked)

Python

7212 readers
1 users here now

Welcome to the Python community on the programming.dev Lemmy instance!

πŸ“… Events

PastNovember 2023

October 2023

July 2023

August 2023

September 2023

🐍 Python project:
πŸ’“ Python Community:
✨ Python Ecosystem:
🌌 Fediverse
Communities
Projects
Feeds

founded 2 years ago
MODERATORS
23
How is my Python code? (raw.githubusercontent.com)
submitted 1 month ago* (last edited 1 month ago) by [email protected] to c/[email protected]
 

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

top 24 comments
sorted by: hot top controversial new old
[–] [email protected] 11 points 1 month ago (2 children)

You appear to be afraid of spaces. Everything is cramped together.

Just bc you don't have to put a space after colons or after equals and commas and whatnot doesn't mean you shouldn't

Don't be afraid of spaces.

They make it easier for you to read your code when you come back later

[–] [email protected] 3 points 1 month ago (2 children)

Oops, sorry. I will revise the code and place spaces. Thanks for suggestion <3

[–] [email protected] 6 points 1 month ago* (last edited 1 month ago) (1 children)

Usually, you would use a formatter anyway - it's good to know the standard way but for day to day coding I just have a shortcut bound that runs ruff format (you can even have it done automatically on file save).

[–] [email protected] 1 points 1 month ago (1 children)

I need to search formatters. Thank you for suggestion.

[–] [email protected] 5 points 1 month ago (1 children)

Not the original commenter, but no need to apologise my friend. Nice work. Learning tip from me: give PEP 8 a read and save it for reference somewhere. It's the standard for how to format Python code, and future you will thank you for internalising it early on in your Python journey

[–] [email protected] 1 points 1 month ago

Thank you I will read it.

[–] [email protected] 1 points 1 month ago (1 children)

I revised the code. Added some spaces and comments for better readability. Hope it's better now.

[–] [email protected] 3 points 1 month ago

Instead you should just use an autoformatter. Black is good. Ruff is probably good too but I don't have a lot of experience with it. YAPF is not good; don't use it.

[–] [email protected] 9 points 1 month ago (1 children)

It's mostly ok, my immediate reaction is that it's unnecessary to put the whole program into a class. Python isn't Java. It's fine to write procedures at the top level.

Also as someone said, consider using enums, and also the new case statement as of Python 3.10. Type annotations also can make things clearer. I've been using mypy to check mine but there are some alternatives now that might be better.

There is not much error checking but that can be ok if you are the only user.

If the config file might be crafted maliciously, it can use escape codes for a terminal playback attack: https://www.cyberark.com/resources/threat-research-blog/dont-trust-this-title-abusing-terminal-emulators-with-ansi-escape-characters

Be careful of that.

[–] [email protected] 1 points 1 month ago (1 children)

Yes I realized that I put everything to two classes. It's too much. I'll make more classes for more attributes.

I didn't know enums until now and it's awesome. I will do some exercises about it.

Last thing is interesting. I need to read that. Thank you for suggestions.

[–] [email protected] 2 points 1 month ago

There's no need to make more classes. You can move some of the code so it's not in any class.

[–] [email protected] 7 points 1 month ago (1 children)

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 :)

[–] [email protected] 2 points 1 month ago

That's very informative, I will rewrite the code with your suggestions. Thank you!

[–] [email protected] 1 points 1 month ago

Pretty good for a week and a half! I would recommend setting up a project using uv and also adding type hints and check them with Pyright (NOT Mypy).

[–] danielquinn 1 points 1 month ago* (last edited 1 month ago) (2 children)

I actually really like doing code reviews, so I'm happy to dig into this one. I have a lot of comments, but please don't take this as a criticism of your skills, or even of the code itself. I'm just... really thorough ;-) Hopefully, you'll find some of this helpful.

Formatting

The first suggestion I have is to install a code formatter like ruff or black. The Python community develops our code to a standard called pep8 which you're violating all over the place here, making it harder to read your code. Rather than having to go through it all and manually conforming it though, tools like ruff or black will do it with one command. A lot of developers will even plug them into their IDE to run against their code on-save, so you don't even have to think about running it.

CONSTANTS vs attributes

Pep8 says that constants should be in ALL_CAPS while attributes should be in snake_case, and for the most part, it looks like you're trying to conform to that, but some of your constants aren't... well constant. You define self.API_URL for example as the result of a variable passed to __init__. This would mean that the value could conceivably be different in two instances of the same class. There's nothing wrong with that, but labelling it as if it were a constant is bad form.

Type Hints

While not necessary for running your program, they're still super helpful both for readability and ensuring you haven't introduced any bugs accidentally. If you're using a proper IDE like PyCharm for example, it will warn you if you've passed an int to a method that's expecting a string for example. As your project gets more complicated, and you start passing around complex objects with varying abilities, type hints can be a big help.

Classes

Don't listen to the haters on classes. This is great. It's a good clean way to maintain state inside a component, rather than having to pass objects around to disparate functions.

The Superfluous else

This is just a pet peeve of mine, but else should be used sparingly. There's nothing wrong with it per se, but it's often unnecessary. Consider the following two toy examples:

def my_func() -> None:
    if something:
        return 4
    else:
        return 5

That else is useless. the above could be:

def my_func() -> None:
    if something:
        return 4
    return 5

Similarly, this:

def my_func() -> None:
    if something:
        x = 1
        y = 2
    else:
        raise SomeException(...)

This would be a lot clearer if you invert the test:

def my_func() -> None:
    if not something:
        raise SomeException(...)

    x = 1
    y = 2

This comes up all the time, and it just bugs me. I'm not throwing shade, just trying to reduce the number of superfluous else's in the world :-)

Complex dicts for data

The big offender here is WeatherApp.LOCATIONS. You define it first as a constant that you later change (see above), and you reference parts of it all over the place, meaning you've got to ensure that every part of your app (a) is familiar with its internals and (b) won't modify it, thereby breaking other parts of your app accidentally. There's also this LOWLOCATIONS business, which reads like a crutch you added to work around other limitations.

Instead, I suggest delegating this sort of thing to a separate class. Consider something like this:

from typing import Self
 
 
class Location:
    def __init__(self, name: str, lat: float, lng: float) -> None:
        self.name = name
        self.lat = lat
        self.lng = lng


class LocationManager:
    def __init__(self) -> None:
        self._locations = []
    
    def add_location(self, name: str, lat: float, lng: float) -> None:
        self._locations.append(Location(name=name, lat=lat, lng=lng))
    
    @classmethod
    def build(cls, data: dict[str, dict[str, float]]) -> Self:
        self = cls()
        for name, coords in data.items():
            self.add_location(name, coords["lat"], coords["lng"])
        return self
    
    def export(self) -> dict[str, dict[str, float]]:
        r = {}
        for loc in self._locations:
            r[loc.name] = {"lat": loc.lat, "lng": loc.lng}
        return r

This would offload the complexity carrying around that structure throughout your application and instead define a set API between "how you think about locations" and "how your app uses them". It'd also let you remove a lot of your location-specific code from the rest of your project where arguably it doesn't belong.

Defining attritributes in __init__

It's fine to change an instance's attributes from inside a random method, but initialising them should only ever happen in __init__. This is largely to do with readability, but also with stability. Consider the following:

class Thing:
    def __init__(self) -> None:
        pass

    def do_x(self, something: str) -> None:
        self.alpha = something

    def do_y(self) -> None:
        print(self.alpha)

Thing().do_y()

This will barf with an attribute error and when someone looks at the class trying to figure out where alpha is set, __init__() is useless. Now imagine that the class is around 600 lines long and that's quite the headache.

Sometimes, this is rather academic, like where you've called .load_config() from .__init__() directly. In these cases, I'd recommend defining the attribute on the class so at least it's clear that this is something that the class supports externally:

class WeatherApp:
    config: dict[str, str | dict[str, dict[str, float]
    api_url: str
    locations: LocationManager

    def __init__(self, config_file="config.json") -> None:
        self.load_config(config_file)

Delegation

It's great that you're trying to break up your project into responsible parts, but they're rather tightly coupled, so the responsibility to do anything rests with the entire app rather than any individual part.

For example, you've go this WeatherAPI class. It's a good idea. You want to delegate the responsibility of talking to an external API to something that knows how to do that (and only that), but in order to use the one you've built here, I'd need to pass it both the name of the city you want to look up, and the entire cities dataset, and the URL it needs to do the job.

It's kind of like asking someone to make you a sandwich, and then taking the time to show them where the bread, ham, and cheese are in the fridge... you might as well make it yourself at that point.

Additionally, your API abstraction isn't just doing API things, but it's also handling the rendering step, doing a whole bunch of print() and colour formatting etc. That's not the job of an API.

Instead, I suggest building this sort of thing "backwards". Decide on the API you want, and then build that out. Imagine for a moment how you might want the WeatherAPI class to work:

class WeatherApp:
    def __init__(self, ...) -> None
        ...
        self.wapi  = WeatherAPI(api_url)

    def run(self) -> None:
        ...
        for location in locations:
            # We can use our `Location` class to pass 
            # everything `.fetch()` needs in one variable.
            weather = wapi.fetch(location)
            self.render_daily_weather(weather.current)
            self.render_current_temperature(weather.temperature)

This drastically simplifies your WeatherAPI class and allows you room to add support for things like caching, or handling rate limits etc. Better yet, you can delegate the various bits of data tweaking you're doing in both WeatherApp and WeatherAPI to a separate class that just accepts raw API data and then distils it down to useful values. In the end, it might look something like this:

class WeatherResult:

    CODES = {
        0: "Clear sky",
        ...
    }

    DAY_NIGHT = {0: "Day", 1: "Night"}

    def __init__(self, current: dict, temperature: float) -> None:
        self.current = current
        self.temperature = temperature

    @property
    def description(self) -> str:
        return self.CODES.get(
            self.current["weather_code"],
            "Unknown weather code",
        )

    @property
    def daynight(self) -> str:
        return self.DAY_NIGHT[self.current["is_day"]]


class WeatherAPI:

    def __init__(self, url: str) -> None:
        self.url = url

    def fetch(self, city: Location) -> None:
        url = f"{self.url}?latitude={city.lat}&longitude"
        ...
        return WeatherResult(current=..., temperature=...)

That's it! Honestly, this is a good start, so please don't be discouraged by the verbosity of the above. Thanks for sharing your code, and have fun in future projects!

[–] [email protected] 2 points 1 month ago

That's quite a detailed review. Thank you for your attention! I will try everything in here <3

[–] [email protected] 2 points 1 month ago (1 children)

Hey, just randomly stumbled across this post and wanted to praise your detailed feedback. It's people like you that make communities great!

[–] danielquinn 1 points 1 month ago

Aww! Thanks!

[–] Arkouda 1 points 1 month ago (1 children)

I am pretty new myself, but if you didn't know, here are some great free resources to help you.

https://inventwithpython.com/

[–] [email protected] 2 points 1 month ago (1 children)

These are looking awesome. Thank you for sharing this <3

[–] Arkouda 2 points 1 month ago

No problem at all! I hope it helps :)

[–] [email protected] 1 points 1 month ago* (last edited 1 month ago) (1 children)

Too lazy to check the logic but if possible, consider using enums.

[–] [email protected] 2 points 1 month ago

Yes I need to use enums. I realized it now.