Crthuky

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)

• 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 consolidates somehow. There are a lot of repeated self statements and similar functions, making the code harder to follow and likely 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 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."

  
  


• 
  

  When incrementing by 1:

  
  
  

  
  

  card_counter += 1
  

  
  

  
  
  

  You can just use the ++ operator:

  
  
  

  card_counter++
  

  
  

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)

However, that exposes a larger issue - you shouldn't be keeping copies of the same card value in your deck data structure. Instead, consider something like a dictionary, whose keys are the card names, and the values are card counts. That would really simplify much of your code.

Don't eval

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