Return to Revisions

I'd say in general, it is quite decent code. Nothing written too complicated, variables are named so that I understand it and 'stuff' which belongs together is mostly separated within classes.

Some java coding conventions:

• spaces after commas (e.g. Demo, creation of Combatant), in front of opening curly brackets, ...
• interfaces: Do not need public modifier in methods, they are public per se
• else is on the same line as the closing curly bracket
• best you check the code conventions, usually IDE's do have a formatter template (-> always format your code)

Comments

• Comments do not end with '//'.
• Comments such as 'instance variables' or 'methods' are considered "clutter". There are conventions and guidelines on how order your code.

Combatant

• maxHealth is always 10 and never gets changed, so you might want to declare that as a constant.
• The takeTurn method calls performAction. I personally prefer smaller methods over one large and therefore complicated methods, but I also prefer one small method over two small methods. And since performAction is only called by takeTurn, I don't see any reason why not call battleAction within the takeTurn method.
• decreaseHealth / increaseHealth: Formatting ...

Actions

• toString is already a method in the Object class. It is either not needed, if you want to call toString, but I usually prefer a separate method.
• The Actions are displaying text and I don't think that's their job. Just think about not having a console client, or let's say, a window ui and console client and how you'd solve the displaying of data in that case.
• The execute method have two Combatant parameters. But a self-heal doesn't work like that. So you basically have to pass a target in the HealAction, but you don't use it. I think in that case, it is better to pass the needed parameters to the constructor of the actions. And also maybe there will be a 'group attack' action, the interface therefore cannot do that, since it needs n targets.

BattleHelper

• The do while is a bit complicated to understand. I think, first of all, I'd rename i to action. And then, move the reading of said action to a separate method. Then you just have your sysos, something like readAction, and then the if/else
• Instead of if/else, you can do a switch-case, so you end up with a default...
• Also, assume you have 100 possible actions, you will have 100 else-if's. You might want to think about how to solve that problem.