Skip to main content
added 926 characters in body
Source Link
Reinderien
  • 71.2k
  • 5
  • 76
  • 256
  • Don't inherit from (object); Python 2 needs to die (and you already rely on Python 3 given your current code)
  • An OO approach would have a Suit class (even if it's a lightweight named tuple) with properties of, perhaps: symbol, name, letter, colour. This is a little less awkward than is_red. These properties can then be factored away from Card, and only a reference to a suit object kept for each card.
  • Having the color dictionary on Card doesn't make much sense; you'll want to have a module or class that takes care of screen operations such as clear, and ANSI escape sequences for colour etc.
  • The pattern Card.color['r'] + self.symbol + r + Card.color['w'] calls out for a utility or two. One pattern you could follow is a @contextmanager that sets the colour, runs the inner code, and then clears the colour after (even if an exception is thrown).
  • You have some typehints - good! But they're inconsistent. For instance,
    def get_symbol(self, colored=0) -> str:

needs a hint for colored. Since you asked, your hand=None should be hand: Optional[List[Card]] = None.

  • Your Blackjack constructor shouldn't be responsible for running the entire game; just initialisation.
  • The call self.dealer.__init__() is concerning. Presumably that object is already initialised via Hand(), so - calling it again is surprising. If you just want to replace the instance, replace the instance.
  • length, is_busted etc. are a good fit to be @propertys. The purpose of a property is not particularly to do a character-by-character code length trade; it's to communicate to callers of your class that certain values from your class are available without needing to pass parameters, ideally without mutating the class, and without a whole lot of computation. Properties are also convenient when using modern debuggers as they appear without having to be explicitly called.
  • You have some manually-formatted currency strings. locale.currency is made for this.
  • self.bankroll // 1000 // 1000 * 1000 seems like you're doing a little bit of trickery to get a round effect. That's less obvious than just calling round. You say I was trying to give two different min_bet for users. $5 for someone with less than $1 million bankroll, and roughly 0.1% of bankroll rounded down to a thousand dollars for someone with a million or above bankroll. Clear code for this behaviour is important (also, don't name a variable d):
if self.bankroll < 1e6:
    default = self.min_bet
else:
    default = round(self.bankroll/1000, -3)
  • user_input < lo or user_input > hi is equivalent to not (lo <= user_input <= hi)
  • Apparently this is controversial, but my opinion is that $100m is ten cents. $100M is 100 million dollars.
  • Don't inherit from (object); Python 2 needs to die (and you already rely on Python 3 given your current code)
  • An OO approach would have a Suit class (even if it's a lightweight named tuple) with properties of, perhaps: symbol, name, letter, colour. This is a little less awkward than is_red. These properties can then be factored away from Card, and only a reference to a suit object kept for each card.
  • Having the color dictionary on Card doesn't make much sense; you'll want to have a module or class that takes care of screen operations such as clear, and ANSI escape sequences for colour etc.
  • The pattern Card.color['r'] + self.symbol + r + Card.color['w'] calls out for a utility or two. One pattern you could follow is a @contextmanager that sets the colour, runs the inner code, and then clears the colour after (even if an exception is thrown).
  • You have some typehints - good! But they're inconsistent. For instance,
    def get_symbol(self, colored=0) -> str:

needs a hint for colored.

  • Your Blackjack constructor shouldn't be responsible for running the entire game; just initialisation.
  • The call self.dealer.__init__() is concerning. Presumably that object is already initialised via Hand(), so - calling it again is surprising. If you just want to replace the instance, replace the instance.
  • length, is_busted etc. are a good fit to be @propertys.
  • You have some manually-formatted currency strings. locale.currency is made for this.
  • self.bankroll // 1000 // 1000 * 1000 seems like you're doing a little bit of trickery to get a round effect. That's less obvious than just calling round.
  • user_input < lo or user_input > hi is equivalent to not (lo <= user_input <= hi)
  • Apparently this is controversial, but my opinion is that $100m is ten cents. $100M is 100 million dollars.
  • Don't inherit from (object); Python 2 needs to die (and you already rely on Python 3 given your current code)
  • An OO approach would have a Suit class (even if it's a lightweight named tuple) with properties of, perhaps: symbol, name, letter, colour. This is a little less awkward than is_red. These properties can then be factored away from Card, and only a reference to a suit object kept for each card.
  • Having the color dictionary on Card doesn't make much sense; you'll want to have a module or class that takes care of screen operations such as clear, and ANSI escape sequences for colour etc.
  • The pattern Card.color['r'] + self.symbol + r + Card.color['w'] calls out for a utility or two. One pattern you could follow is a @contextmanager that sets the colour, runs the inner code, and then clears the colour after (even if an exception is thrown).
  • You have some typehints - good! But they're inconsistent. For instance,
    def get_symbol(self, colored=0) -> str:

needs a hint for colored. Since you asked, your hand=None should be hand: Optional[List[Card]] = None.

  • Your Blackjack constructor shouldn't be responsible for running the entire game; just initialisation.
  • The call self.dealer.__init__() is concerning. Presumably that object is already initialised via Hand(), so - calling it again is surprising. If you just want to replace the instance, replace the instance.
  • length, is_busted etc. are a good fit to be @propertys. The purpose of a property is not particularly to do a character-by-character code length trade; it's to communicate to callers of your class that certain values from your class are available without needing to pass parameters, ideally without mutating the class, and without a whole lot of computation. Properties are also convenient when using modern debuggers as they appear without having to be explicitly called.
  • You have some manually-formatted currency strings. locale.currency is made for this.
  • self.bankroll // 1000 // 1000 * 1000 seems like you're doing a little bit of trickery to get a round effect. That's less obvious than just calling round. You say I was trying to give two different min_bet for users. $5 for someone with less than $1 million bankroll, and roughly 0.1% of bankroll rounded down to a thousand dollars for someone with a million or above bankroll. Clear code for this behaviour is important (also, don't name a variable d):
if self.bankroll < 1e6:
    default = self.min_bet
else:
    default = round(self.bankroll/1000, -3)
  • user_input < lo or user_input > hi is equivalent to not (lo <= user_input <= hi)
  • Apparently this is controversial, but my opinion is that $100m is ten cents. $100M is 100 million dollars.
Source Link
Reinderien
  • 71.2k
  • 5
  • 76
  • 256

  • Don't inherit from (object); Python 2 needs to die (and you already rely on Python 3 given your current code)
  • An OO approach would have a Suit class (even if it's a lightweight named tuple) with properties of, perhaps: symbol, name, letter, colour. This is a little less awkward than is_red. These properties can then be factored away from Card, and only a reference to a suit object kept for each card.
  • Having the color dictionary on Card doesn't make much sense; you'll want to have a module or class that takes care of screen operations such as clear, and ANSI escape sequences for colour etc.
  • The pattern Card.color['r'] + self.symbol + r + Card.color['w'] calls out for a utility or two. One pattern you could follow is a @contextmanager that sets the colour, runs the inner code, and then clears the colour after (even if an exception is thrown).
  • You have some typehints - good! But they're inconsistent. For instance,
    def get_symbol(self, colored=0) -> str:

needs a hint for colored.

  • Your Blackjack constructor shouldn't be responsible for running the entire game; just initialisation.
  • The call self.dealer.__init__() is concerning. Presumably that object is already initialised via Hand(), so - calling it again is surprising. If you just want to replace the instance, replace the instance.
  • length, is_busted etc. are a good fit to be @propertys.
  • You have some manually-formatted currency strings. locale.currency is made for this.
  • self.bankroll // 1000 // 1000 * 1000 seems like you're doing a little bit of trickery to get a round effect. That's less obvious than just calling round.
  • user_input < lo or user_input > hi is equivalent to not (lo <= user_input <= hi)
  • Apparently this is controversial, but my opinion is that $100m is ten cents. $100M is 100 million dollars.