Yu-Gi-Oh cards in Python 3





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







12












$begingroup$


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)









share|improve this question











$endgroup$












  • $begingroup$
    What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
    $endgroup$
    – flakes
    Apr 1 at 0:22


















12












$begingroup$


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)









share|improve this question











$endgroup$












  • $begingroup$
    What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
    $endgroup$
    – flakes
    Apr 1 at 0:22














12












12








12


2



$begingroup$


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)









share|improve this question











$endgroup$




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)






python python-3.x playing-cards






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Mar 31 at 2:31









Jamal

30.6k11121227




30.6k11121227










asked Mar 31 at 2:10









Jerry CuiJerry Cui

115111




115111












  • $begingroup$
    What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
    $endgroup$
    – flakes
    Apr 1 at 0:22


















  • $begingroup$
    What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
    $endgroup$
    – flakes
    Apr 1 at 0:22
















$begingroup$
What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
$endgroup$
– flakes
Apr 1 at 0:22




$begingroup$
What you really need are some unit tests. I see some pretty noticable bugs in the Deck class, that a simple test would highlight
$endgroup$
– flakes
Apr 1 at 0:22










5 Answers
5






active

oldest

votes


















17












$begingroup$

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.






share|improve this answer











$endgroup$









  • 3




    $begingroup$
    I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
    $endgroup$
    – Quelklef
    Mar 31 at 10:33






  • 1




    $begingroup$
    Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
    $endgroup$
    – Ilmari Karonen
    Mar 31 at 20:00










  • $begingroup$
    I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
    $endgroup$
    – Jasper
    Apr 1 at 11:18



















12












$begingroup$



  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.






share|improve this answer









$endgroup$





















    8












    $begingroup$

    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.






    share|improve this answer








    New contributor




    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.






    $endgroup$





















      8












      $begingroup$


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








      share|improve this answer











      $endgroup$









      • 3




        $begingroup$
        ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
        $endgroup$
        – jingyu9575
        Mar 31 at 8:01












      • $begingroup$
        @jingyu9575: I was already aware of that (maybe I didn't word it as such).
        $endgroup$
        – Jamal
        Mar 31 at 15:44



















      5












      $begingroup$

      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.






      share|improve this answer








      New contributor




      dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






      $endgroup$














        Your Answer





        StackExchange.ifUsing("editor", function () {
        return StackExchange.using("mathjaxEditing", function () {
        StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
        StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
        });
        });
        }, "mathjax-editing");

        StackExchange.ifUsing("editor", function () {
        StackExchange.using("externalEditor", function () {
        StackExchange.using("snippets", function () {
        StackExchange.snippets.init();
        });
        });
        }, "code-snippets");

        StackExchange.ready(function() {
        var channelOptions = {
        tags: "".split(" "),
        id: "196"
        };
        initTagRenderer("".split(" "), "".split(" "), channelOptions);

        StackExchange.using("externalEditor", function() {
        // Have to fire editor after snippets, if snippets enabled
        if (StackExchange.settings.snippets.snippetsEnabled) {
        StackExchange.using("snippets", function() {
        createEditor();
        });
        }
        else {
        createEditor();
        }
        });

        function createEditor() {
        StackExchange.prepareEditor({
        heartbeatType: 'answer',
        autoActivateHeartbeat: false,
        convertImagesToLinks: false,
        noModals: true,
        showLowRepImageUploadWarning: true,
        reputationToPostImages: null,
        bindNavPrevention: true,
        postfix: "",
        imageUploader: {
        brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
        contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
        allowUrls: true
        },
        onDemand: true,
        discardSelector: ".discard-answer"
        ,immediatelyShowMarkdownHelp:true
        });


        }
        });














        draft saved

        draft discarded


















        StackExchange.ready(
        function () {
        StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216560%2fyu-gi-oh-cards-in-python-3%23new-answer', 'question_page');
        }
        );

        Post as a guest















        Required, but never shown

























        5 Answers
        5






        active

        oldest

        votes








        5 Answers
        5






        active

        oldest

        votes









        active

        oldest

        votes






        active

        oldest

        votes









        17












        $begingroup$

        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.






        share|improve this answer











        $endgroup$









        • 3




          $begingroup$
          I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
          $endgroup$
          – Quelklef
          Mar 31 at 10:33






        • 1




          $begingroup$
          Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
          $endgroup$
          – Ilmari Karonen
          Mar 31 at 20:00










        • $begingroup$
          I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
          $endgroup$
          – Jasper
          Apr 1 at 11:18
















        17












        $begingroup$

        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.






        share|improve this answer











        $endgroup$









        • 3




          $begingroup$
          I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
          $endgroup$
          – Quelklef
          Mar 31 at 10:33






        • 1




          $begingroup$
          Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
          $endgroup$
          – Ilmari Karonen
          Mar 31 at 20:00










        • $begingroup$
          I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
          $endgroup$
          – Jasper
          Apr 1 at 11:18














        17












        17








        17





        $begingroup$

        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.






        share|improve this answer











        $endgroup$



        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.







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited Mar 31 at 19:08

























        answered Mar 31 at 4:15









        ReinderienReinderien

        5,250926




        5,250926








        • 3




          $begingroup$
          I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
          $endgroup$
          – Quelklef
          Mar 31 at 10:33






        • 1




          $begingroup$
          Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
          $endgroup$
          – Ilmari Karonen
          Mar 31 at 20:00










        • $begingroup$
          I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
          $endgroup$
          – Jasper
          Apr 1 at 11:18














        • 3




          $begingroup$
          I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
          $endgroup$
          – Quelklef
          Mar 31 at 10:33






        • 1




          $begingroup$
          Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
          $endgroup$
          – Ilmari Karonen
          Mar 31 at 20:00










        • $begingroup$
          I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
          $endgroup$
          – Jasper
          Apr 1 at 11:18








        3




        3




        $begingroup$
        I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
        $endgroup$
        – Quelklef
        Mar 31 at 10:33




        $begingroup$
        I would argue that the first point is a manner of style. I personally prefer writing it like you have suggested, but sometimes I do it the other way to be explicit.
        $endgroup$
        – Quelklef
        Mar 31 at 10:33




        1




        1




        $begingroup$
        Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
        $endgroup$
        – Ilmari Karonen
        Mar 31 at 20:00




        $begingroup$
        Nice edits. I would still suggest self.main_deck.count(card_to_add) over sum(1 for card in self.main_deck if card == card_to_add), though.
        $endgroup$
        – Ilmari Karonen
        Mar 31 at 20:00












        $begingroup$
        I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
        $endgroup$
        – Jasper
        Apr 1 at 11:18




        $begingroup$
        I'd guess the eval might be because the card data isn't coming from code files (but, for example, a set of external files defining what cards exist). The idea of being able to update the card database without updating code is always an alluring one for a trading card game. Of course, eval still probably isn't a good choice, but that would mean there are no "easy" fixes like the one you described.
        $endgroup$
        – Jasper
        Apr 1 at 11:18













        12












        $begingroup$



        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.






        share|improve this answer









        $endgroup$


















          12












          $begingroup$



          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.






          share|improve this answer









          $endgroup$
















            12












            12








            12





            $begingroup$



            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.






            share|improve this answer









            $endgroup$





            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.







            share|improve this answer












            share|improve this answer



            share|improve this answer










            answered Mar 31 at 16:59









            PeilonrayzPeilonrayz

            26.5k338111




            26.5k338111























                8












                $begingroup$

                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.






                share|improve this answer








                New contributor




                user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                Check out our Code of Conduct.






                $endgroup$


















                  8












                  $begingroup$

                  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.






                  share|improve this answer








                  New contributor




                  user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.






                  $endgroup$
















                    8












                    8








                    8





                    $begingroup$

                    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.






                    share|improve this answer








                    New contributor




                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.






                    $endgroup$



                    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.







                    share|improve this answer








                    New contributor




                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.









                    share|improve this answer



                    share|improve this answer






                    New contributor




                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.









                    answered Mar 31 at 6:36









                    user10987432user10987432

                    812




                    812




                    New contributor




                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.





                    New contributor





                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.






                    user10987432 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                    Check out our Code of Conduct.























                        8












                        $begingroup$


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








                        share|improve this answer











                        $endgroup$









                        • 3




                          $begingroup$
                          ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                          $endgroup$
                          – jingyu9575
                          Mar 31 at 8:01












                        • $begingroup$
                          @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                          $endgroup$
                          – Jamal
                          Mar 31 at 15:44
















                        8












                        $begingroup$


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








                        share|improve this answer











                        $endgroup$









                        • 3




                          $begingroup$
                          ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                          $endgroup$
                          – jingyu9575
                          Mar 31 at 8:01












                        • $begingroup$
                          @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                          $endgroup$
                          – Jamal
                          Mar 31 at 15:44














                        8












                        8








                        8





                        $begingroup$


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








                        share|improve this answer











                        $endgroup$




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









                        share|improve this answer














                        share|improve this answer



                        share|improve this answer








                        edited Apr 1 at 12:15









                        Mast

                        7,59763788




                        7,59763788










                        answered Mar 31 at 2:50









                        JamalJamal

                        30.6k11121227




                        30.6k11121227








                        • 3




                          $begingroup$
                          ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                          $endgroup$
                          – jingyu9575
                          Mar 31 at 8:01












                        • $begingroup$
                          @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                          $endgroup$
                          – Jamal
                          Mar 31 at 15:44














                        • 3




                          $begingroup$
                          ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                          $endgroup$
                          – jingyu9575
                          Mar 31 at 8:01












                        • $begingroup$
                          @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                          $endgroup$
                          – Jamal
                          Mar 31 at 15:44








                        3




                        3




                        $begingroup$
                        ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                        $endgroup$
                        – jingyu9575
                        Mar 31 at 8:01






                        $begingroup$
                        ATK and DEF are probably considered special terminology in Yu-Gi-Oh.
                        $endgroup$
                        – jingyu9575
                        Mar 31 at 8:01














                        $begingroup$
                        @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                        $endgroup$
                        – Jamal
                        Mar 31 at 15:44




                        $begingroup$
                        @jingyu9575: I was already aware of that (maybe I didn't word it as such).
                        $endgroup$
                        – Jamal
                        Mar 31 at 15:44











                        5












                        $begingroup$

                        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.






                        share|improve this answer








                        New contributor




                        dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                        Check out our Code of Conduct.






                        $endgroup$


















                          5












                          $begingroup$

                          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.






                          share|improve this answer








                          New contributor




                          dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                          Check out our Code of Conduct.






                          $endgroup$
















                            5












                            5








                            5





                            $begingroup$

                            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.






                            share|improve this answer








                            New contributor




                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.






                            $endgroup$



                            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.







                            share|improve this answer








                            New contributor




                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.









                            share|improve this answer



                            share|improve this answer






                            New contributor




                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.









                            answered Mar 31 at 15:38









                            dudenr33dudenr33

                            1943




                            1943




                            New contributor




                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.





                            New contributor





                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.






                            dudenr33 is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                            Check out our Code of Conduct.






























                                draft saved

                                draft discarded




















































                                Thanks for contributing an answer to Code Review Stack Exchange!


                                • Please be sure to answer the question. Provide details and share your research!

                                But avoid



                                • Asking for help, clarification, or responding to other answers.

                                • Making statements based on opinion; back them up with references or personal experience.


                                Use MathJax to format equations. MathJax reference.


                                To learn more, see our tips on writing great answers.




                                draft saved


                                draft discarded














                                StackExchange.ready(
                                function () {
                                StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216560%2fyu-gi-oh-cards-in-python-3%23new-answer', 'question_page');
                                }
                                );

                                Post as a guest















                                Required, but never shown





















































                                Required, but never shown














                                Required, but never shown












                                Required, but never shown







                                Required, but never shown

































                                Required, but never shown














                                Required, but never shown












                                Required, but never shown







                                Required, but never shown







                                Popular posts from this blog

                                Plaza Victoria

                                Puebla de Zaragoza

                                Musa