After Dave gave you some hints about increasing the style and stability of your code I think I can give you some advises concerning your design.
In your method proximoComando() you do the same thing over and over which practically screams for refactoring:
You check the Direccion you chose at the beginning and if your check succeeds you return an action. The design patterns the instantly spring to my mind to clean this up are Strategy and Chain of Responsibility.
Here we go...
First we use the strategy pattern to abstract the checks of the battleship's surroundings...
public interface SituationCheck {
// check the situation and return true if the concrete conditions are met
boolean isConditionMet(CabinaDeControl cabina, Direccion contactDireccion, Civilizacion ownCivilizacion);
}
... and the same for the actions that the pilot can perform...
public interface PilotAction {
// return the command that corresponds to the given action
Comando issueCommand(Direccion[] direccions, Direccion contactDireccion, Control control);
}
now we can use these interfaces to model the behaviour of the battleship following the Chain-of-Responsibility pattern. I named the class Orders since it represents a set of orders what your pilot has to do in specific situations:
public class Orders {
private SituationCheck check = null;
private PilotAction action = null;
private Orders next = null;
public Orders(SituationCheck check, PilotAction action) {
this.check = check;
this.action = action;
this.next = null;
}
public Orders(PilotAction action) {
this(null, action);
}
/**
* Performs situation checks and issues the appropriate command
*
* @param contactDireccion
* the direction that is checked
* @param cabina
* the radar report of a given direccion
* @param ownCivilizacion
* the ship's own civilizacion
* @return the command the fits to the situation
*/
public Comando checkSituationAndIssueCommand(Direccion[] direccions, Direccion contactDireccion, CabinaDeControl cabina, Civilizacion ownCivilizacion) {
if (check == null || check.isConditionMet(cabina, contactDireccion, ownCivilizacion)) {
if (action != null) { // adding null actions should be avoided ;)
return action.issueCommand(direccions, contactDireccion, cabina.getControl());
}
} else if (next != null) {
return next.checkSituationAndIssueCommand(direccions, contactDireccion, cabina, ownCivilizacion);
}
// if we reach the end of the chain and no situation check succeeded
// or if there was no action assigned to the situation
// this may be replaced by a dummy action e.g. DoNothing
return null;
}
public Orders add(Orders orders) {
if (next == null) {
next = orders;
return next;
} else {
return next.add(orders);
}
}
public Orders add(SituationCheck newCheck, PilotAction newAction) {
return add(new Orders(newCheck, newAction));
}
public void add(PilotAction finalAction) {
add(new Orders(finalAction));
}
}
I'll explain that step by step:
Each order consist of three things
- a check that will applied to the given situation
- an action that will be performed if the check succeeds and
- another order that is consider if the check fails (i.e. the next part in our chain)
There are two constructors
- one with a check and an action to create a normal part of the chain
- one with only an action that marks the end of our chain
I wrote few methods to add a new element to the chain. I let two of them return the current end of the chain so we can easily put it together. If you don't like this make them void methods.
Two create the behaviour of your example we need a few concrete checks ...
public class CheckForEspectro implements SituationCheck {
private Espectro espectro;
public CheckForEspectro(Espectro espectro) {
this.espectro = espectro;
}
@Override
public boolean isConditionMet(CabinaDeControl cabina, Direccion contactDireccion, Civilizacion ownCivilizacion) {
return cabina.isEspectro(espectro, contactDireccion);
}
}
public class CheckForEnemy implements SituationCheck {
private Espectro enemyType; // e.g. BASE, NAVE
public CheckForEnemy(Espectro enemyType) {
this.enemyType = enemyType;
}
@Override
public boolean isConditionMet(CabinaDeControl cabina, Direccion contactDireccion, Civilizacion ownCivilizacion) {
return cabina.isEspectro(enemyType, contactDireccion) && cabina.contactIsHostileTo(contactDireccion, ownCivilizacion);
}
}
public class CheckForAlly implements SituationCheck {
private Espectro allyType; // e.g. BASE, NAVE
public CheckForAlly(Espectro allyType ) {
this.allyType = allyType ;
}
@Override
public boolean isConditionMet(CabinaDeControl cabina, Direccion contactDireccion, Civilizacion ownCivilizacion) {
return cabina.isEspectro(allyType, contactDireccion) && cabina.contactIsAlliedTo(contactDireccion, ownCivilizacion);
}
}
public class CheckForContainer implements SituationCheck {
private Sustancia sustancia;
public CheckForContainer(Sustancia sustancia) {
this.sustancia = sustancia;
}
@Override
public boolean isConditionMet(CabinaDeControl cabina, Direccion contactDireccion, Civilizacion ownCivilizacion) {
return cabina.isEspectro(Espectro.CONTAINER, contactDireccion) && cabina.contactContainsSubstance(contactDireccion, sustancia);
}
}
... and actions ....
public class Attack implements PilotAction {
@Override
public Comando issueCommand(Direccion[] direccions, Direccion contactDireccion, Control control) {
return control.atacar(contactDireccion);
}
}
public class MoveToRandomPosition implements PilotAction {
@Override
public Comando issueCommand(Direccion[] direccions, Direccion contactDireccion, Control control) {
int index = (int) Math.round(Math.random() * direccions.length);
return control.avanzar(direccions[index]);
}
}
public class Wait implements PilotAction {
@Override
public Comando issueCommand(Direccion[] direccions, Direccion contactDireccion, Control control) {
return control.esperar();
}
}
public class Transfer implements PilotAction {
public enum Direction {
UPLOAD, DOWNLOAD
}
private Direction direction;
private Sustancia sustancia;
private int amount;
public Transfer(Direction direction, Sustancia sustancia, int amount) {
this.direction = direction;
this.sustancia = sustancia;
this.amount = amount;
}
@Override
public Comando issueCommand(Direccion[] direccions, Direccion contactDireccion, Control control) {
switch (direction) {
case UPLOAD:
return control.transferirCarga(contactDireccion, Direccion.ORIGEN, sustancia, amount);
case DOWNLOAD:
return control.transferirCarga(Direccion.ORIGEN, contactDireccion, sustancia, amount);
default: // currently unreachable
return null;
}
}
}
Please note that i assumed that you already implemented some of Dave's ideas e.g. i call a method
cabina.contactIsHostileTo(contactDireccion, ownCivilizacion)
that i assume to return the result of your check
this.cabina.getRadar().getReporte( values[i] ).getCivilizacion()!=this.civilizacion
where contactDireccion coresponds to values[i] and ownCivilizacion to this.civilizacion.
This way you have that logic in exactly one place and can easily exchange that simple check to a more complex one e.g. by evaluating some diplomacy matrix.
Now we can alter your class by adding a field for the Orders
private Orders orders;
and set them up in the constructor:
public PiloPilotoImperial() {
super();
Orders orders = new Orders(new CheckForEspectro(Espectro.DESCONOCIDO), new MoveToRandomPosition());
orders.add(new CheckForEnemy(Espectro.NAVE), new Attack())
.add(new CheckForEnemy(Espectro.BASE), new Attack())
.add(new CheckForContainer(Sustancia.ANTIMATERIA), new Transfer(Direction.UPLOAD, Sustancia.ANTIMATERIA, 100))
.add(new CheckForAlly(Espectro.BASE), new Transfer(Direction.DOWNLOAD, Sustancia.ANTIMATERIA, 100))
.add(new CheckForEspectro(Espectro.VACACIO), new MoveToRandomPosition())
.add(new Wait());
}
public PilotoImperial(Imperio civilizacion) {
this();
this.civilizacion = civilizacion;
}
and finally your method will be much shorter:
public Comando proximoComando() {
Direccion[] direccions = Direccion.values();
if (direccions == null || direccions.length == 0) {
return null;
}
Direccion contactDireccion = direccions[Math.round(Math.random() * values.length-1)];
return orders.checkSituationAndIssueCommand(direccions, contactDireccion, getCabina(), getCivilizacion());
}
I hope this answer was not too long but i wanted to show you how you could rewrite your code by dividing it into little independent pieces that are easy to understand can be put together to form a more complex structure in many different variations.
For example you could use different combinations of checks and actions to create a freighter that transfers other stuff and flees from enemies.
You can find my source code here - i added some dummy implementations of your classes to avoid compiler errors. feel free to alter anything you want.
this.cabina.getRadar().getReporte(values[i]).getEspectro()andthis.cabina.getControl()\$\endgroup\$