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

Author: douglascdev

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)

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:
+        ...

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

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"

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

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),
                     ]
                 )
             )

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

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:

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