muhammadhasan01 / avatarduelgamecard Goto Github PK

I noticed that the factory classes that are created only return the new instance of the card class. I recommend reading these references about Factory Builder [1] [2]. It would be nice to only have one factory class that could make those three cards.

This issue is only a suggestion. You may ignore it (not all the suggestions are true), challenge it, or come up with another idea yourself.

https://github.com/muhammadhasan01/tugasBesarOOP2/blob/8259a75a84734ed7e8d1636c78d31c66ea6d6986/AvatarDuel/src/main/java/com/avatarduel/cards/CharacterCards.java#L43-L46

Instead of IOException, maybe consider a more detailed exception like IndexOutOfBoundsException?

This issue is only a suggestion. You may ignore it (not all the suggestions are true), challenge it, or come up with another idea yourself.

https://github.com/muhammadhasan01/tugasBesarOOP2/blob/8259a75a84734ed7e8d1636c78d31c66ea6d6986/AvatarDuel/src/main/java/com/avatarduel/gameboard/CardHand.java#L15-L27

I am curious about this. Why create a CardHand class if the constructors and method only its parent's?

There are repeated methods in CharacterCards, LandCards, SkillCards. It would be nice to consider converting these list-of-cards classes into an generic abstract class so only loadCards() method is implemented.

P.S. maybe loadCards implementation can also be reduced so only Row.forEach() is needed inside loadCards?

This issue is only a suggestion. You may ignore it (not all the suggestions are true), challenge it, or come up with another idea yourself.