Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added guess the number game #1405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

sarzz2
Copy link

@sarzz2 sarzz2 commented Dec 4, 2023

Relevant Issues

Closes #1082

Description

Added new cog of guess the number game

Did you:

@Xithrius Xithrius self-requested a review December 4, 2023 08:38
@Xithrius Xithrius added type: feature Relating to the functionality of the application. area: backend Related to internal functionality and utilities status: needs review Author is waiting for someone to review and approve category: fun Related to fun and games labels Dec 4, 2023
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is purely a code review for now.
I will test once the changes are implemented and the conversion bug is fixed.

bot/exts/fun/guess_number.py Show resolved Hide resolved
bot/exts/fun/guess_number.py Show resolved Hide resolved
bot/exts/fun/guess_number.py Show resolved Hide resolved
await ctx.send("Timeout!")
return

if not user_guess.content.isdigit():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isdigit would break on some instances as it takes unicode into account.

For example, if you call '²'.isdigit(), it'll return True.
Instead, wrap it in a try.except block where you try to convert it into an integer by calling int(content)

return

if not user_guess.content.isdigit():
await ctx.send(f"**Invalid guess**, Please guess a number. Try again, You've {tries} left.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would look better in an Embed IMO.

bot/exts/fun/guess_number.py Show resolved Hide resolved
return

if not user_guess.content.isdigit():
await ctx.send(f"**Invalid guess**, Please guess a number. Try again, You've {tries} left.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure here whether we should just quit on invalid numbers, or otherwise set a counter for the number of invalid tries a user can have, otherwise this could keep on going forever. I think both are fine.

What i'm sure of though, is that we shouldn't let a backdoor that'll keep the command running forever.

if not user_guess.content.isdigit():
await ctx.send(f"**Invalid guess**, Please guess a number. Try again, You've {tries} left.")

elif int(user_guess.content) == self.active_games[ctx.author.id]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should make the conversion only once, before anything else, then keep on using the converted number if successfull. The conversion should be done like I indicated in my previous comment.

else:
tries = tries - 1
if tries == 0:
await ctx.channel.send(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go into the while loop's else block, which separates the end game code from the rest & makes it clearer & less indented.

@wookie184 wookie184 added status: waiting for author Waiting for author to address a review or respond to a comment and removed status: needs review Author is waiting for someone to review and approve labels Jan 28, 2024
@Xithrius
Copy link
Member

Xithrius commented Feb 2, 2024

@sarzz2 Hey, just checking up. Will you be continuing this PR?

Thanks!

@sarzz2
Copy link
Author

sarzz2 commented Feb 2, 2024

@sarzz2 Hey, just checking up. Will you be continuing this PR?

Thanks!

Yes, been a little busy with work lately. Planning to do it by next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to internal functionality and utilities category: fun Related to fun and games status: waiting for author Waiting for author to address a review or respond to a comment type: feature Relating to the functionality of the application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add number guessing game
5 participants