From fa3f2edfaa0ce1251fffe4fa6bfc29c8e157e63a Mon Sep 17 00:00:00 2001 From: Douglas Date: Mon, 9 Jan 2023 22:01:37 -0300 Subject: [PATCH] Code quality improvements (#39) * Remove unnecessary and poorly coded "point reward for chatting" functionality * Remove unused imports from Bank * Create BankUser protocol with withdraw and deposit methods * Add name method to BankUser protocol * Implement BankUser protocol method name() * Use dependency inversion on Bank's BotUser dependency using the BankUser protocol, decoupling it from BotUser * Make command batch fail on any Exception, not just ValueError * Test banking module * Add get_balance method to BankUser protocol * Use dependency inversion on PointAmount's BotUser dependency * Put test bankuser in a separate module for reuse * Rename exception raised by point converter, validate maximum number of digits and if bank_user is passed for all non-numeric amounts * Test point amount converter * Fix undefined reference on translation module --- hashtablebot/banking/bank.py | 24 ------ hashtablebot/banking/bank_user.py | 15 ++++ hashtablebot/banking/commands.py | 44 +++++----- hashtablebot/bot_exceptions.py | 2 +- hashtablebot/entity/bot_user.py | 7 ++ hashtablebot/hash_table_bot.py | 31 +++---- hashtablebot/memory_entity/point_amount.py | 45 ++++++---- hashtablebot/translation.py | 2 +- tests/bank_user_test.py | 27 ++++++ tests/test_banking.py | 98 ++++++++++++++++++++++ tests/test_point_amount.py | 48 ++++++++++- 11 files changed, 256 insertions(+), 87 deletions(-) create mode 100644 hashtablebot/banking/bank_user.py create mode 100644 tests/bank_user_test.py create mode 100644 tests/test_banking.py diff --git a/hashtablebot/banking/bank.py b/hashtablebot/banking/bank.py index 0fc8974..82e0d16 100644 --- a/hashtablebot/banking/bank.py +++ b/hashtablebot/banking/bank.py @@ -1,11 +1,4 @@ -from sqlalchemy.exc import NoResultFound -from twitchio import Chatter - -from hashtablebot.banking.commands import Deposit from hashtablebot.banking.transaction import Transaction -from hashtablebot.entity.bot_user import BotUser -from hashtablebot.persistence.bot_user_dao import BotUserDao -from hashtablebot.user_checks import is_chatter class Bank: @@ -39,20 +32,3 @@ def redo(self) -> None: transaction = self.redo_stack.pop() transaction.execute() self.undo_stack.append(transaction) - - def reward_chatter(self, chatter: Chatter, reward: int): - if reward <= 0 or not is_chatter(chatter): - return - - author_id = int(chatter.id) - - try: - author_bot_user: BotUser = BotUserDao.get_by_id(author_id) - except NoResultFound: - # TODO: I guess this breaks single responsibility principle...? Should probably refactor it - author_bot_user = BotUser(id=author_id) - - self.execute(Deposit(bot_user=author_bot_user, amount=reward)) - - # TODO: should probably remove this too, saving should not happen here - BotUserDao.save(author_bot_user) diff --git a/hashtablebot/banking/bank_user.py b/hashtablebot/banking/bank_user.py new file mode 100644 index 0000000..9ab2605 --- /dev/null +++ b/hashtablebot/banking/bank_user.py @@ -0,0 +1,15 @@ +from typing import Protocol + + +class BankUser(Protocol): + def withdraw(self, amount: int): + ... + + def deposit(self, amount: int): + ... + + def name(self) -> str: + ... + + def get_balance(self) -> int: + ... diff --git a/hashtablebot/banking/commands.py b/hashtablebot/banking/commands.py index b22e813..e9c83a4 100644 --- a/hashtablebot/banking/commands.py +++ b/hashtablebot/banking/commands.py @@ -1,80 +1,80 @@ import logging from dataclasses import dataclass, field +from hashtablebot.banking.bank_user import BankUser from hashtablebot.banking.transaction import Transaction -from hashtablebot.entity.bot_user import BotUser @dataclass class Deposit: - bot_user: BotUser + bank_user: BankUser amount: int @property def transfer_details(self) -> str: - return f"${self.amount} to bot_user {self.bot_user.id}" + return f"${self.amount} to bot_user {self.bank_user.name()}" def execute(self) -> None: - self.bot_user.deposit(self.amount) + self.bank_user.deposit(self.amount) logging.info(f"Deposited {self.transfer_details}") def undo(self) -> None: - self.bot_user.withdraw(self.amount) + self.bank_user.withdraw(self.amount) logging.info(f"Undid deposit of {self.transfer_details}") def redo(self) -> None: - self.bot_user.deposit(self.amount) + self.bank_user.deposit(self.amount) logging.info(f"Redid deposit of {self.transfer_details}") @dataclass class Withdrawal: - bot_user: BotUser + bank_user: BankUser amount: int @property def transfer_details(self) -> str: - return f"${self.amount/100} from bot_user {self.bot_user.id}" + return f"${self.amount/100} from bot_user {self.bank_user.name()}" def execute(self) -> None: - self.bot_user.withdraw(self.amount) + self.bank_user.withdraw(self.amount) logging.info(f"Withdrawn {self.transfer_details}") def undo(self) -> None: - self.bot_user.deposit(self.amount) + self.bank_user.deposit(self.amount) logging.info(f"Undid withdrawal of {self.transfer_details}") def redo(self) -> None: - self.bot_user.withdraw(self.amount) + self.bank_user.withdraw(self.amount) logging.info(f"Redid withdrawal of {self.transfer_details}") @dataclass class Transfer: - from_bot_user: BotUser - to_bot_user: BotUser + from_bank_user: BankUser + to_bank_user: BankUser amount: int @property def transfer_details(self) -> str: return ( - f"${self.amount} from bot_user {self.from_bot_user.id}" - f" to bot_user {self.to_bot_user.id}" + f"${self.amount} from bot_user {self.from_bank_user.name()}" + f" to bot_user {self.to_bank_user.name()}" ) def execute(self) -> None: - self.from_bot_user.withdraw(self.amount) - self.to_bot_user.deposit(self.amount) + self.from_bank_user.withdraw(self.amount) + self.to_bank_user.deposit(self.amount) logging.info(f"Transferred {self.transfer_details}") def undo(self) -> None: - self.to_bot_user.withdraw(self.amount) - self.from_bot_user.deposit(self.amount) + self.to_bank_user.withdraw(self.amount) + self.from_bank_user.deposit(self.amount) logging.info(f"Undid transfer of {self.transfer_details}") def redo(self) -> None: - self.from_bot_user.withdraw(self.amount) - self.to_bot_user.deposit(self.amount) + self.from_bank_user.withdraw(self.amount) + self.to_bank_user.deposit(self.amount) logging.info(f"Redid transfer of {self.transfer_details}") @@ -88,7 +88,7 @@ def execute(self) -> None: for command in self.commands: command.execute() completed_commands.append(command) - except ValueError: + except Exception: for command in reversed(completed_commands): command.undo() raise diff --git a/hashtablebot/bot_exceptions.py b/hashtablebot/bot_exceptions.py index ace7df6..3496eb4 100644 --- a/hashtablebot/bot_exceptions.py +++ b/hashtablebot/bot_exceptions.py @@ -17,7 +17,7 @@ def get_chat_message() -> str: return "You don't have enough elis coins elisLookingAtYou" -class InvalidPointAmountError(ExceptionWithChatMessage): +class PointConversionError(ExceptionWithChatMessage): @staticmethod def get_chat_message() -> str: return "An invalid point amount was passed elisLookingAtYou" diff --git a/hashtablebot/entity/bot_user.py b/hashtablebot/entity/bot_user.py index 1c0d1e6..44707a8 100644 --- a/hashtablebot/entity/bot_user.py +++ b/hashtablebot/entity/bot_user.py @@ -43,3 +43,10 @@ def withdraw(self, amount: int): raise NotEnoughCoinError("Not enough funds") self.balance -= amount + + def name(self) -> str: + # TODO: add name attribute to bot_user + return str(self.id) + + def get_balance(self) -> int: + return self.balance diff --git a/hashtablebot/hash_table_bot.py b/hashtablebot/hash_table_bot.py index f06f33a..f3dc21c 100644 --- a/hashtablebot/hash_table_bot.py +++ b/hashtablebot/hash_table_bot.py @@ -14,8 +14,8 @@ from hashtablebot.banking.commands import Batch, Deposit, Withdrawal from hashtablebot.bot_exceptions import ( ExceptionWithChatMessage, - InvalidPointAmountError, NotEnoughCoinError, + PointConversionError, ) from hashtablebot.entity.bot_user import BotUser from hashtablebot.memory_entity.no_prefix_command import DefaultNoPrefix @@ -137,13 +137,6 @@ async def event_message(self, message: Message): and not message.content.startswith(await self._prefix(self, message)) ) if message_is_not_command: - # Distribute point reward for chatting - try: - Bank().reward_chatter(message.author, self._chatting_message_reward) - - except Exception as e: - logging.exception(e) - await self._translator.translate_user_message(message) elif not invoked_no_prefix_command: @@ -281,17 +274,17 @@ async def gamble(self, ctx: commands.Context, amount_str: str): try: amount = PointAmountConverter.convert(amount_str, author) - except InvalidPointAmountError as e: + except PointConversionError as e: await ctx.reply(e.get_chat_message()) logging.exception(e) return try: if random.randint(0, 1): - Bank().execute(Deposit(bot_user=author, amount=amount)) + Bank().execute(Deposit(bank_user=author, amount=amount)) message = f"{ctx.author.name} won {amount} coins elisCoin !!! POGGERS They now have {author.balance}" else: - Bank().execute(Withdrawal(bot_user=author, amount=amount)) + Bank().execute(Withdrawal(bank_user=author, amount=amount)) message = f"{ctx.author.name} lost {amount} coins... peepoSad They now have {author.balance}" except ExceptionWithChatMessage as e: @@ -409,7 +402,7 @@ async def give(self, ctx: commands.Context, target_user: User, amount_str: str): try: amount = PointAmountConverter.convert(amount_str, author_bot_user) - except InvalidPointAmountError as e: + except PointConversionError as e: logging.exception(e) await ctx.reply(e.get_chat_message()) return @@ -425,8 +418,8 @@ async def give(self, ctx: commands.Context, target_user: User, amount_str: str): Bank().execute( Batch( [ - Withdrawal(bot_user=author_bot_user, amount=amount), - Deposit(bot_user=target_bot_user, amount=amount), + Withdrawal(bank_user=author_bot_user, amount=amount), + Deposit(bank_user=target_bot_user, amount=amount), ] ) ) @@ -499,7 +492,7 @@ async def duel(self, ctx: commands.Context, duel_target: User, amount_str: str): try: amount = PointAmountConverter.convert(amount_str, author_bot_user) - except InvalidPointAmountError as e: + except PointConversionError as e: logging.exception(e) await ctx.reply(e.get_chat_message()) return @@ -583,8 +576,8 @@ def store_answer(msg: Message): Bank().execute( Batch( [ - Deposit(bot_user=author_bot_user, amount=amount), - Withdrawal(bot_user=duel_target_bot_user, amount=amount), + Deposit(bank_user=author_bot_user, amount=amount), + Withdrawal(bank_user=duel_target_bot_user, amount=amount), ] ) ) @@ -596,8 +589,8 @@ def store_answer(msg: Message): Bank().execute( Batch( [ - Withdrawal(bot_user=author_bot_user, amount=amount), - Deposit(bot_user=duel_target_bot_user, amount=amount), + Withdrawal(bank_user=author_bot_user, amount=amount), + Deposit(bank_user=duel_target_bot_user, amount=amount), ] ) ) diff --git a/hashtablebot/memory_entity/point_amount.py b/hashtablebot/memory_entity/point_amount.py index c7f1084..0e8b69f 100644 --- a/hashtablebot/memory_entity/point_amount.py +++ b/hashtablebot/memory_entity/point_amount.py @@ -1,5 +1,5 @@ -from hashtablebot.bot_exceptions import InvalidPointAmountError -from hashtablebot.entity.bot_user import BotUser +from hashtablebot.banking.bank_user import BankUser +from hashtablebot.bot_exceptions import PointConversionError class PointAmountConverter: @@ -11,47 +11,56 @@ class PointAmountConverter: """ @staticmethod - def convert(amount: str, bot_user: BotUser) -> int: + def convert(amount: str, bank_user: BankUser) -> int: """ Convert an amount of points from any format to an integer value. - :param bot_user: + :param bank_user: :param amount: string with a percentage, number or word describing an amount - :raises: InvalidPointAmountError + :raises: PointConversionError if one of these conditions is met: + - the amount passed is invalid + - bank_user is not passed for a numeric amount + - amount exceeds 20 digits """ + if len(amount) > 20: + raise PointConversionError("Amount exceeds 20 digits!") + if amount.isnumeric(): return PointAmountConverter._convert_from_num(amount) + + if not bank_user: + raise PointConversionError( + "Non-numeric amounts require a BankUser to be passed!" + ) + elif amount.endswith("%"): - return PointAmountConverter._convert_from_percentage(amount, bot_user) + return PointAmountConverter._convert_from_percentage(amount, bank_user) else: - return PointAmountConverter._convert_from_keyword(amount, bot_user) + return PointAmountConverter._convert_from_keyword(amount, bank_user) @staticmethod def _convert_from_num(amount: str) -> int: try: int_amount = int(amount) except ValueError: - raise InvalidPointAmountError("An invalid amount of points was passed!") + raise PointConversionError("An invalid amount of points was passed!") else: return int_amount @staticmethod - def _convert_from_percentage(amount: str, bot_user: BotUser) -> int: + def _convert_from_percentage(amount: str, bank_user: BankUser) -> int: try: int_percentage = int(amount[:-1]) except ValueError: - raise InvalidPointAmountError("An invalid percentage was passed!") + raise PointConversionError("An invalid percentage was passed!") else: - if not bot_user: - raise InvalidPointAmountError("Not a valid user!") - - return (bot_user.balance * int_percentage) // 100 + return (bank_user.get_balance() * int_percentage) // 100 @staticmethod - def _convert_from_keyword(amount: str, bot_user: BotUser) -> int: + def _convert_from_keyword(amount: str, bank_user: BankUser) -> int: match amount: case "all": - return bot_user.balance + return bank_user.get_balance() case "half": - return bot_user.balance // 2 + return bank_user.get_balance() // 2 case _: - raise InvalidPointAmountError("Not a valid amount") + raise PointConversionError("Not a valid amount") diff --git a/hashtablebot/translation.py b/hashtablebot/translation.py index 5b4aab4..28f1fb0 100644 --- a/hashtablebot/translation.py +++ b/hashtablebot/translation.py @@ -130,7 +130,7 @@ async def translate_user_message(self, message: Message) -> None: logging.debug( f"Translating '{message.content}' from '{user.source_lang}' to '{user.target_lang}'" ) - translated_msg = tss.google( + translated_msg = tss.server.google( message.content, user.source_lang, user.target_lang ) except Exception as e: diff --git a/tests/bank_user_test.py b/tests/bank_user_test.py new file mode 100644 index 0000000..fbcc48b --- /dev/null +++ b/tests/bank_user_test.py @@ -0,0 +1,27 @@ +from dataclasses import dataclass + +from hashtablebot.bot_exceptions import NotEnoughCoinError + + +@dataclass +class BankUserTest: + """ + Test implementation of BankUser Protocol + """ + + balance: int + + def deposit(self, amount: int): + self.balance += amount + + def withdraw(self, amount: int): + if amount > self.balance: + raise NotEnoughCoinError("Not enough funds") + + self.balance -= amount + + def name(self) -> str: + return "Name" + + def get_balance(self) -> int: + return self.balance diff --git a/tests/test_banking.py b/tests/test_banking.py new file mode 100644 index 0000000..f6836a3 --- /dev/null +++ b/tests/test_banking.py @@ -0,0 +1,98 @@ +from unittest import TestCase + +from hashtablebot.banking.bank import Bank +from hashtablebot.banking.bank_user import BankUser +from hashtablebot.banking.commands import Batch, Deposit, Transfer, Withdrawal +from tests.bank_user_test import BankUserTest + + +class TestBanking(TestCase): + """ + Tests for Banking module + """ + + def setUp(self) -> None: + self.bank_user: BankUser = BankUserTest(balance=100) + + def test_deposit_batch(self): + Bank().execute( + Batch( + [ + Deposit(bank_user=self.bank_user, amount=100), + Deposit(bank_user=self.bank_user, amount=100), + ] + ) + ) + self.assertEqual(self.bank_user.get_balance(), 300) + + def test_withdraw_batch(self): + Bank().execute( + Batch( + [ + Withdrawal(bank_user=self.bank_user, amount=10), + Withdrawal(bank_user=self.bank_user, amount=10), + ] + ) + ) + self.assertEqual(self.bank_user.get_balance(), 80) + + def test_undo_redo_batch(self): + bank = Bank() + bank.execute( + Batch( + [ + Withdrawal(bank_user=self.bank_user, amount=10), + Withdrawal(bank_user=self.bank_user, amount=10), + ] + ) + ) + bank.undo() + self.assertEqual(self.bank_user.get_balance(), 100) + + bank.redo() + self.assertEqual(self.bank_user.get_balance(), 80) + + def test_redo_with_no_transactions(self): + bank = Bank() + + # Redo stack is empty, should keep the same value + bank.redo() + self.assertEqual(self.bank_user.get_balance(), 100) + + def test_undo_with_no_transactions(self): + bank = Bank() + + # Undo stack is empty, should keep the same value + bank.undo() + self.assertEqual(self.bank_user.get_balance(), 100) + + def test_failing_transaction_reverses_batch(self): + with self.assertRaises(Exception): + Bank().execute( + Batch( + [ + # Executes + Withdrawal(bank_user=self.bank_user, amount=50), + # Fails, should reverse previous transaction + Withdrawal(bank_user=self.bank_user, amount=51), + ] + ) + ) + self.assertEqual(self.bank_user.get_balance(), 100) + + def test_transfer(self): + transfer_target: BankUser = BankUserTest(balance=100) + + bank = Bank() + + bank.execute(Transfer(self.bank_user, transfer_target, 100)) + self.assertEqual(self.bank_user.get_balance(), 0) + self.assertEqual(transfer_target.get_balance(), 200) + + bank.undo() + self.assertEqual(self.bank_user.get_balance(), 100) + self.assertEqual(transfer_target.get_balance(), 100) + + bank.redo() + self.assertEqual(self.bank_user.get_balance(), 0) + self.assertEqual(transfer_target.get_balance(), 200) diff --git a/tests/test_point_amount.py b/tests/test_point_amount.py index 89f0ab2..a1a5edd 100644 --- a/tests/test_point_amount.py +++ b/tests/test_point_amount.py @@ -1,6 +1,50 @@ from unittest import TestCase +from hashtablebot.banking.bank_user import BankUser +from hashtablebot.bot_exceptions import PointConversionError +from hashtablebot.memory_entity.point_amount import PointAmountConverter +from tests.bank_user_test import BankUserTest + class TestPointAmountConverter(TestCase): - def test_convert(self): - pass + """ + Test the point converter + """ + + def setUp(self) -> None: + self.bank_user: BankUser = BankUserTest(balance=100) + + def test_convert_num(self): + self.assertEqual(PointAmountConverter.convert("10", self.bank_user), 10) + + # Fails above 20 digits + with self.assertRaises(PointConversionError): + PointAmountConverter.convert("".join("1" * 21), self.bank_user) + + # Doesn't fail on 20 digits + max_points = "".join("1" * 20) + self.assertEqual( + PointAmountConverter.convert(max_points, self.bank_user), int(max_points) + ) + + def test_convert_percent(self): + self.assertEqual(PointAmountConverter.convert("10%", self.bank_user), 10) + + self.bank_user.withdraw(55) # balance: 45 + self.assertEqual(PointAmountConverter.convert("10%", self.bank_user), 4) + + with self.assertRaises(PointConversionError): + PointAmountConverter.convert("10%", None) + + with self.assertRaises(PointConversionError): + PointAmountConverter.convert("a%", self.bank_user) + + def test_convert_keyword(self): + self.assertEqual(PointAmountConverter.convert("all", self.bank_user), 100) + self.assertEqual(PointAmountConverter.convert("half", self.bank_user), 50) + + with self.assertRaises(PointConversionError): + PointAmountConverter.convert("asdasf", self.bank_user) + + with self.assertRaises(PointConversionError): + PointAmountConverter.convert("all", None)