Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. See more controversy about Builder at http://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern https://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. See more controversy about Builder at http://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. See more controversy about Builder at https://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.

added 114 characters in body
Source Link

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may or may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. ThatSee more controversy about Builder at http://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may or may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. That said, I think you implemented it fine.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. See more controversy about Builder at http://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.

added notes about Builder
Source Link

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may or may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. That said, I think you implemented it fine.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

For the Main.battle() you could refactor a lot:

  • Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
  • Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
  • The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
  • dragonX and dragonY are more readable than x and y
  • printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
  • Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

  • Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

    Separate the construction of a complex object from its representation so that the same construction process can create different representations.

    What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may or may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. That said, I think you implemented it fine.

Source Link
Loading
lang-java

AltStyle によって変換されたページ (->オリジナル) /