Trjtdtk

I made a project in Python 3.7.1 that includes classes to create Yu-Gi-Oh cards. This isn't the entire game; it is just the cards itself. I will create the entire game later. I want feedback on how to improve my code.

class Deck(object):
 def __init__(self, main_deck):
 self.main_deck = main_deck

 def add_normal_cards(self, card_to_add, all_cards):
 """
 Add monsters, spells and traps to the deck.
 """
 if len(self.main_deck) > 60:
 return "You have to many cards in your deck (60)."
 else:
 card_counter = 0
 # Check to see how many copies of a card there are in your deck. Maximum 3 of the same card. Limited cards
 # will be added eventually.
 for card in self.main_deck:
 if card == card_to_add:
 card_counter += 1
 if card_counter == 3:
 return "You have to many copies of that card in your deck (3)."
 else:
 if card_to_add not in all_cards:
 return "That card hasn't been added to the game yet (" + card_to_add + ")."
 else:
 self.main_deck.append(card_to_add)

 def add_extra_cards(self, card_to_add, all_cards):
 """
 Add monsters, spells and traps to the deck.
 """
 if len(self.main_deck) > 15:
 return "You have to many cards in your extra deck (15)."
 else:
 card_counter = 0
 # Check to see how many copies of a card there are in your deck. Maximum 3 of the same card. Limited cards
 # will be added eventually.
 for card in self.main_deck:
 if card == card_to_add:
 card_counter += 1
 if card_counter == 3:
 return "You have to many copies of that card in your deck (3)."
 else:
 if card_to_add not in all_cards:
 return "That card hasn't been added to the game yet (" + card_to_add + ")."
 else:
 self.main_deck.append(card_to_add)


class Monster(object):
 def __init__(self, name, effects, attributes, monster_type, atk, _def, description):
 self.name = name
 self.effects = effects
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self._def = _def
 self.description = description

 def effect(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)


class Spell(object):
 def __init__(self, name, effects):
 self.name = name
 self.effects = effects

 def activate(self):
 """
 Activate the effect of this spell.
 """
 for effect in self.effects:
 eval(effect)


class Trap(object):
 def __init__(self, name, effects):
 self.name = name
 self.effects = effects

 def activate(self):
 """
 Activate the effect of this spell.
 """
 for effect in self.effects:
 eval(effect)


class LinkMonster(object):
 def __init__(self, name, effects, attributes, monster_type, atk, link_rating, description, recipe, links):
 self.name = name
 self.effects = effects
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self.link_rating = link_rating
 self.description = description
 self.recipe = recipe
 self.links = links

 def activate(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)


class SynchroMonster(object):
 def __init__(self, name, effects, attributes, monster_type, atk, _def, description, recipe):
 self.name = name
 self.effects = effects
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self._def = _def
 self.description = description
 self.recipe = recipe

 def activate(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)


class XyzMonster(object):
 def __init__(self, name, effects, attributes, monster_type, atk, _def, description, recipe):
 self.name = name
 self.effects = effects
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self._def = _def
 self.description = description
 self.recipe = recipe
 self.materials = recipe

 def activate(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)


class FusionMonster(object):
 def __init__(self, name, effects, attributes, monster_type, atk, _def, description, recipe):
 self.name = name
 self.effects = effects
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self._def = _def
 self.description = description
 self.recipe = recipe

 def activate(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)


class PendulumMonster(object):
 def __init__(self, name, effects, pendulum_effects, pendulum_scale, attributes, monster_type, atk, _def,
 description, recipe):
 self.name = name
 self.effects = effects
 self.pendulum_effects = pendulum_effects
 self.pendulum_scale = pendulum_scale
 self.attributes = attributes
 self.type = monster_type
 self.atk = atk
 self._def = _def
 self.description = description
 self.recipe = recipe
 self.materials = recipe

 def activate(self):
 """
 Activate the effect of this monster.
 """
 for effect in self.effects:
 eval(effect)

 def pendulum_activate(self):
 """
 Activate the effect of this monster while in Spell/Trap Zone.
 """
 for effect in self.pendulum_effects:
 eval(effect)

Trim redundant else

...here:

if len(self.main_deck) > 60:
 return "You have to many cards in your deck (60)."
else:

The else isn't needed because you've already returned. This pattern happens in a few places.

Lose some loops

This:

 card_counter = 0
 for card in self.main_deck:
 if card == card_to_add:
 card_counter += 1

can be

card_counter = sum(1 for card in self.main_deck if card == card_to_add)

If this happens often, you may want to do some preprocessing in a different method to make this easier. As in the comments, it shouldn't replace the sequential-format deck, but it could supplement it:

from collections import Counter
card_counts = Counter(main_deck)
# ...
card_counter = card_counts[card_to_add]

Don't eval

Just don't. There's never a good reason. Make effects an iterable of functions, and simply call them.

1. 
   

   dataclasses.dataclass would single handedly remove the majority of your code.

   
   
   

   from dataclasses import dataclass
   from typing import List
   
   
   @dataclass
   class Monster(object):
    name: str
    effects: List[str]
    attributes: str
    type: str
    atk: int
    def_: int
    description: str
   
    def effect(self):
    """
    Activate the effect of this monster.
    """
    for effect in self.effects:
    eval(effect)
   

   
   


2. 
   

   Your classes are setup for inheritance.

   
   
   

   class FusionMonster(Monster):
    pass
   

   
   



3. I'd use composition over inheritance. This as equip monster cards can go into multiple types. It should be noted that things like Relinquished can make any monster an equipable.



4. 
   

   Your current effect method doesn't care about any of the timing rules. This wiki page seems pretty complete on explaining the edge cases. I remember having this happen a couple times, IIRC it was because of my opponent playing an Iron Chain deck.

   
   
   

   IIRC to do this you'd want to make a 'chain' class that is a stack, you then put all of the effects onto the stack. Once you have built the stack you then run backwards through the chain to to resolve the effects. (I.e. build with Stack.append and resolve with Stack.pop.)

   
   
   

   A rudimentary example would be a D.D. deck vs a Frog deck.

   
   
   

   Say I use my Dupe frog to attack and kill itself to one of your monsters, to send it to the graveyard to start up my combo. If dimensional fissure was a quick spell. After I declare my attack, you could use DF, if that's the end of the chain then DFs effect would activate. Then Dupe frog wouldn't be sent to the graveyard and its timing would be missed.

   
   



5. Yugioh has a lot of infinite loops, and so you should design the chain class to take these into affect too.

I think taking these factors into account at the start are very important as it'll force you to implement your code in the correct manner.

Since you're using python 3.7, I would take advantage of f-strings.
For example, this:

return "That card hasn't been added to the game yet (" + card_to_add + ")."

Becomes:

return f"That card hasn't been added to the game yet (card_to_add)."

As others have said, avoid hardcoding magic numbers like 60, 3 etc.
Give these numbers meaningful names so that it is immediately obvious
what the numbers represent.

Also, unless I'm missing something critical, those monster classes are begging to use inheritance.

• I know you're trying to abbreviate "defense," but you could consider writing the whole thing out instead of making inconsistent naming with the preceding underscore. As such, you could write out "attack" as well to keep it more consistent.


• Perhaps all those monster classes should be part of a common base class inheritance. There are a lot of repeated self statements and similar functions (especially activate()), making the code harder to follow and maintain.


• 
  

  The 60, 15, and 3 are "magic numbers" (numbers without some kind of given context), and from where they are, they could be harder to maintain.

  
  
  

  Although Python doesn't have actual constants like other languages, you can still do something similar above the functions:

  
  
  

  MAX_DECK_CARDS = 60
  

  
  
  
  
  

  MAX_EXTRA_CARDS = 15
  

  
  
  
  
  

  MAX_EXTRA_CARDS = 3
  

  
  


• 
  

  There's a grammatical error here:

  
  
  

  
  

  "You have to many copies of that card in your deck (3)."
  

  
  

  
  
  

  It should be "too" instead of "to."

  
  

In your class Deck the methods add_normal_card and add_extra_cards share a lot of duplicated code which only differs in the maximum number of allowed cards and the error message displayed if this number is exceeded.

You could pull out the else path in an own method.

Also I was a bit confused about the attribute main_deck which is passed in __init__:

• since the class itself is already named Deck one could assume that main_deck is also an instance of some kind of custom Deck class, while it is just a list. This could be clarified by picking another name (like list_of_cards), adding a docstring to __init__ or using type hints.


• 
  add_extra_cards checks the size of main_deck but returns the error message "You have to many cards in your extra deck (15)." I would assume that an extra deck and main deck are separate instances. Is this a bug?

Last but not least the error handling of add_normal_cards and add_extra_cards can be improved. Right now they simply return None if all went well (which is OK), but if some of your conditions like maximum desk size are not met you simply return a str.

Think about the caller of your methods and how he or she should handle those errors.
With your current implementation, they would need to check if the returned object is not None and then compare string values to determine what happened and react to it.
This is error prone because if you decide to change the phrasing of your return values the caller's code will break.

Instead, you should raise a meaningful exception. Since you are dealing with three potential problems, you should define two custom exception classes, like DeckSizeExceeded and CardCountExceeded.

The last possible error (card_to_add not in all_cards) could simply lead to an IndexError, so there is no need for a custom exception class here.