Python code review request

[haughty flare]

@flint oar

cattosarus Uploaded Some Code

Hello. I'm a python beginner working on a twitch bot, would like to get some code review from someone who is good at python. I want to keep my code clean, readable and corrent, of course 🙂
This project works fine, hovewer, i have used some new things to me, like dataclasses.
Please tell me which moments can I improve and in which way they should be improved by your opinion

Could upload project as an archive, but probably some people gonna blame me, so just files. Its not big anyways.
Thanks in advance!

1. Windows 10
2. Python 3.9
3. No
4. Currently no
5. Twitch bot

Attachment: requirements.txt

irc==20.1.0
ossapi[async]==3.1.9
twitchio==2.6.0

Attachment: run.py

import logging
from osu_requests.twitch_bot import TwitchBot


LOG_FILENAME = "osu_requests.log"
LOG_FORMAT = "%(asctime)s | %(levelname)s | %(message)s"
LOG_DATE_FORMAT = "%d.%m.%Y %H:%M:%S"

logging.basicConfig(
    format=LOG_FORMAT,
    datefmt=LOG_DATE_FORMAT,
    level=logging.INFO,
    handlers = [
        logging.StreamHandler(),
        logging.FileHandler(LOG_FILENAME, encoding="utf-8")
    ]
)


if __name__ == '__main__':
    bot = TwitchBot()
    bot.run()

Attachment: irc\_bot.py

import asyncio
import logging
from irc.bot import SingleServerIRCBot, ExponentialBackoff
from irc.client import ServerConnection, Event


class IrcBot(SingleServerIRCBot):
    def __init__(self, username, server, port=6667, password=None):
        recon = ExponentialBackoff(min_interval=5, max_interval=30)
        SingleServerIRCBot.__init__(self, [(server, port, password)], username, username, recon=recon)
        self.messages_queue = asyncio.Queue()

    def on_welcome(self, c: ServerConnection, e: Event):
        logging.info(f"Connected to osu!IRC server as {self._nickname}")

    def send_message(self, target: str, text: str):
        target = target.replace(" ", "_")
        self.connection.privmsg(target, text)
        logging.info(f"In-Game message was successfully sent to {target}")

Uploaded these files to a Gist

osu_api.py
twitch_bot.py
settings.py

View The Gist

For safety reasons we do not allow file attachments.

[flint oar]

🤔 also, i dont want this code to be present as gists, could anyone please remove it as soon as my question will be solved?

[hidden crow]

Ok we pull up.

@flint oar What standards do you want me to use for the code review? What's your experience level like?

No point in reviewing your code with the standards I'd have for a senior developer if you're a student who just started out with Python (and the other way around)

[flint oar]

oh im sorry, i don't know any standarts. my experience is just coding bots for different platforms, working with API

yes, im a student beginner

[hidden crow]

Let me rephrase: How harsh do you want the review to be?

[flint oar]

decide by yourself

want to be better, thats the point

[hidden crow]

Alright, I'll give you the full experience then 😛 Don't take anything personally though - some comments might be opinionated, etc. and I'll hold you to a high standard 🙂

[flint oar]

of course, i respect any critics

[hidden crow]

I'll point out good things as well though

[hidden crow]

Here we go:

osu_api.py:

• I really like the use of the logging library
• Clear definition of constants (all caps snake case is great) looks very good
• Precompiled regular expressions probably make sense if they're going to be executed more than once
• Making use of dataclass is nice
• Not a fan of putting all properties in one line, that reduces readability by quite a bit
• Good use of type annotations
• I'd make the None return value more obvious in line 53 by explicitly returning None (instead of just return)
• I'd also add a return None at the end of the parse_beatmap_url function to make it clear that no valid beatmap url was found
• You could immediately return user in line 64 and explicitly return None in the except-block
• Why are you catching a ValueError for other API operations but not in line 74? Seems like this call could raise an exception as well
• Why not immediately return the star rating in line 75?
• Can you ever have an empty match group in line 108? (I'd have to check your regex but I'm lazy so this is your job now)
• Line 127 could probably be a one-liner: return f"+{mods.short_name()}" if mods else ""
• Your type annotation in line 169 is off, if rank is an int and bool(rank) == False, then rank == 0 - no reason to return 0 explicitly here, the check only makes sense if rank is of type Optional[int]

settings.py:

• Makes sense although you might want to store some of this stuff in environment variables

twitch_bot.py:

• Do any of the Twitch API requests raise exceptions?
• Less type annotations here (return types, etc.) - any reason?
• I'd probably break the super long string concat in line 70 up into at least it's own variable

I don't believe your claim that you are a student beginner though, this code looks like it has been written by someone with much more experience 🙂 Maybe you are a student but certainly not a beginner lol

[flint oar]

At shop, will reach to this as soon as return back

okay im back, reading

Precompiled regular expressions probably make sense if they're going to be executed more than once
thanks! they do (really often), each time someone sends a message to the twitch chat, 2 functions are called to search for the bm/profile link, and if bm link is found, then even third runs (for mods)

Not a fan of putting all properties in one line, that reduces readability by quite a bit
im not sure about which style to choose

I'd make the None return value more obvious in line 53 by explicitly returning None (instead of just return)
saw something similar in someone's code but was not understanding why they are writing return None if return makes same result

I'd also add a return None at the end of the parse_beatmap_url function to make it clear that no valid beatmap url was found
same, no understanding, result is the same probably

You could immediately return user in line 64 and explicitly return None in the except-block
does immediate return improve the speed?

Why are you catching a ValueError for other API operations but not in line 74? Seems like this call could raise an exception as well
looking at the docs, i dont see any possible None returning here

Why not immediately return the star rating in line 75?
yes, will be cleaner, thanks

Can you ever have an empty match group in line 108? (I'd have to check your regex but I'm lazy so this is your job now)
i can barely remember i had a list of groups where some of them were either empty string or None (cant remember their type)

Line 127 could probably be a one-liner: return f"+{mods.short_name()}" if mods else ""
it was in the old code, decided to make multi lines because its now a function (so briefly, most functions of this file were inside get_something())

Your type annotation in line 169 is off, if rank is an int and bool(rank) == False, then rank == 0 - no reason to return 0 explicitly here, the check only makes sense if rank is of type Optional[int]
im sorry, cant understand

[hidden crow]

For the properties:

@dataclass
class BeatmapData:
    url: str
    name: str
    star_rating: float
    duration: str
    bpm: float
    status: str
    mirror_url: str

would be clearer in my opinion

[flint oar]

Do any of the Twitch API requests raise exceptions?
they do, for example joining a channel where your bot is banned, but never happened to me and never tried to test. this might break the connect/reconnect functions, if they dont handle it. was never getting any exceptions, my bot is in development since September

Less type annotations here (return types, etc.) - any reason?
you mean i should annotate the None returns where it has no "return" statements?

I'd probably break the super long string concat in line 70 up into at least it's own variable
this is my headache, i would like to have a help with it

I don't believe your claim that you are a student beginner though, this code looks like it has been written by someone with much more experience 🙂 Maybe you are a student but certainly not a beginner lol
yeah, actually im in programming already almost for a year, but my experience is shouting im a beginner

[hidden crow]

You can explicitly return None instead of just return to make it even more obvious you are returning None on purpose (and not because you forgot a variable) - this is just personal preference

Immediately returning is a bit faster technically but this speed difference doesn't matter in the real world (if an additional STORE_FAST + LOAD_FAST instruction solve performance issues in your code Python isn't the right language anyway). Just a little easier to read in my opinion (again opinion based)

As for line 169: This is a minor nitpick, what exactly is the if not rank condition supposed to check against?

[hidden crow]

You could annotate None return types but this just boils down to personal preference

[hidden crow]

I'd believe you if you said you've been working as a developer for a couple of years so definitely not a beginner in my books

[flint oar]

ahh, yes, rank can be None if the user has no statistics for the game mode he choose as his playmode (main gamemode) or his statisctics were wiped because of cheating+unban, or he had no plays in several years. looks like this:

[flint oar]

not my style, so will ignore this advice. i will have to annotate too much None's then

[flint oar]

somehow i got 200 usd paid from american guy in the past, but not working as a developer. main type of work is system administation (more accurately: servers)

[hidden crow]

Valid choice - fair enough

[hidden crow]

So the type should be Optional[int] then I guess

[hidden crow]

So you do have some exposure to IT already 😛

[flint oar]

if i return it as None, then the final string will look like "(emoji) Cattosarus - #None (Ukraine: #None) 8450pp"

here is modified functions to return immediately and return None except of raw returns

class OsuAPIv2(OssapiAsync):
    async def parse_beatmap_url(self, text: str) -> Optional[tuple[Beatmap, Beatmapset]]:
        for url_type, pattern in OSU_BEATMAPS_PATTERNS.items():
            match = re.search(pattern, text)
            if not match:
                continue

            result = match.groups()[-1]
            try:
                if "beatmap_" in url_type:
                    beatmap = await self.beatmap(result)
                    beatmapset = beatmap.beatmapset()
                elif "beatmapset_" in url_type:
                    beatmapset = await self.beatmapset(result)
                    beatmap = beatmapset.beatmaps[0]
                return beatmap, beatmapset
            except ValueError:
                return None
        return None

    async def parse_user_url(self, text: str) -> Optional[User]:
        match = re.search(OSU_USERS_PATTERN, text)
        if not match:
            return None

        result = match.group(3)
        try:
            user = await self.user(result)
            return user
        except ValueError:
            return None

[hidden crow]

Just the type in the parameter, the rest of the logic is perfectly fine

[flint oar]

does python have any f-string magic ways to turn None into integer?

like :.2f for float decimal places or :g for cutting decimals

[hidden crow]

f"{rank or 0}" would work in this case

[flint oar]

f"{data.gamemode} {data.name} - #{data.global_rank or 0} ({data.country}: #{data.country_rank or 0}) {round(data.pp)}pp"```

[hidden crow]

Yup, that would work here (without the need for the formatting function anymore)

[flint oar]

and get rid of format rank function

yeah

        self.irc_bot.messages_queue.put_nowait((
            self.channels[channel.name],
            f"{author.name} » [{data.url} {data.name}] {difficulty_params}"
            f"⏰ {data.duration} ♫ {data.bpm:g} ({data.status}) [{data.mirror_url} mirror dl]"
        ))
``` will strings have a space between them in this scenario?

[hidden crow]

No, these are connected without a space

[flint oar]

also, {mods} can be an empty string and twitch mobile client & osu client dont remove extra spaces. its better to make " +mods" in format function + remove space between to look like {something}{mods} or any better way?

its a minor problem, most people even wont notice, but im worrying about extra spaces

[hidden crow]

What part of the code are you referencing?

[flint oar]

def format_mods(mods: Mod) -> str:
    return f"+{mods.short_name()}" if mods else ""``` ```py
difficulty_params = f"{format_mods(mods)} ★ {data.star_rating:.2f}"``` ```py
f"[{data.status}] {data.name} {difficulty_params}"```

[hidden crow]

I'd keep it as is tbf

[flint oar]

then if no mods, it will look like "something space space something"

[hidden crow]

If there are no mods it'll look like this: [status] {name} * star

[flint oar]

yes, thats what i mean

[hidden crow]

What you could do is:

def format_mods(mods: Mod) -> str:
    return f" +{mods.short_name()} " if mods else " "

difficulty_params = f"{format_mods(mods)}★ {data.star_rating:.2f}"
f"[{data.status}] {data.name}{difficulty_params}"

I think

[flint oar]

then the code became weird

[hidden crow]

Essentially adding the space only if you need it (in the format_mods), not sure if I messed up spacing somewhere but you should get the idea 🗿

[flint oar]

sure

i think its the question to think of later

    async def send_request(self, data: BeatmapData, mods: Mod, author: Union[Chatter, PartialChatter], channel: Channel):
        difficulty_params = f"{format_mods(mods)} ★ {data.star_rating:.2f}"

        twitch_message = f"[{data.status}] {data.name} {difficulty_params}"
        irc_message = (
            f"{author.name} » [{data.url} {data.name}] {difficulty_params} "
            f"⏰ {data.duration} ♫ {data.bpm:g} ({data.status}) [{data.mirror_url} mirror dl]"
        )

        await channel.send(twitch_message)
        self.irc_bot.messages_queue.put_nowait((self.channels[channel.name], irc_message))

    async def send_profile(self, data: UserData, channel: Channel):
        twitch_message = (
            f"{data.gamemode} {data.name} - #{data.global_rank or 0} "
            f"({data.country}: #{data.country_rank or 0}) {round(data.pp)}pp"
        )

        await channel.send(twitch_message)```

good?

[hidden crow]

Looks good to me 🙂

[flint oar]

i guess at this point everything is done, thank you so much! much appreciated talking to you. will come back to this forum with questions if i find some in the future. by the way, what "code smurfing" term does mean?

[hidden crow]

That was a reference to video game smurfing https://urbandictionary.com/define.php?term=smurfing

[flint oar]

you mean clarifying beginner level not being an actual beginner?

[hidden crow]

Yes, your code looks like it was written by someone with quite a bit of experience, as I said 😄

[flint oar]

i can send you some of my python projects if you want via DMs, just to know my level

[hidden crow]

Won't say no to that - although I wouldn't consider you a beginner either way

[flint oar]

as its my hobby, i dont know much about what is trainee, junior, middle, senior and so on (how its measured)

do you want to find the level by looking at other projects?

[hidden crow]

There's no fixed way of measuring experience, some people go off years of experience or whatever but that's not very accurate in my opinion

[hidden crow]

Let's do it!

[flint oar]

oki, let me prepare everything