Ggthjy

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