4
\$\begingroup\$

I made the code, and it works, but it uses many if conditions and it looks ugly. I would really appreciate it if someone could give me a hand by pointing me in the right direction to make it more object-oriented.

package ar.edu.uca.ceis.objetos.imperio;

import ar.uba.fi.algo3.batallaespacial.CabinaDeControl;
import ar.uba.fi.algo3.batallaespacial.Civilizacion;
import ar.uba.fi.algo3.batallaespacial.Piloto;
import ar.uba.fi.algo3.batallaespacial.Reporte.Espectro;
import ar.uba.fi.algo3.batallaespacial.Sustancia;
import ar.uba.fi.algo3.batallaespacial.comandos.Comando;
import ar.uba.fi.algo3.batallaespacial.Direccion;

public class PilotoImperial implements Piloto {

    private CabinaDeControl cabina;
    private Imperio civilizacion;

    public PilotoImperial(Imperio civilizacion) {
        super();
        this.civilizacion = civilizacion;
    }

    public void setCabinaDeControl(CabinaDeControl cabina) {
        this.cabina = cabina;
    }

    public Comando proximoComando() {

     Direccion[] values = Direccion.values();

    // for (int i = 1; i < values.length; i++) {
        int i; 
         i = (int)Math.round(Math.random() *values.length-1) ;

   //if border of an unknow position, go to a random place 
         if (Espectro.DESCONOCIDO == this.cabina.getRadar().getReporte( values[i] ).getEspectro()){             

            // generaremos numeros aleatorios entre 1 y values.length
             int c = (int)Math.round(Math.random() *values.length) ; 

             if (Espectro.VACIO == this.cabina.getRadar().getReporte( values[c] ).getEspectro()){
                    return this.cabina.getControl().avanzar( values[c] );           
                 }                       
         }

   // i found and enemy base, attack     
         if (Espectro.BASE == this.cabina.getRadar().getReporte( values[i] ).getEspectro()) {
               if (this.cabina.getRadar().getReporte( values[i] ).getCivilizacion()!=this.civilizacion){
                   return this.cabina.getControl().atacar(values[i]);             
                   }        
            }

  // if found and enemy, attack 
         if (Espectro.NAVE == this.cabina.getRadar().getReporte( values[i] ).getEspectro()) {
           if (this.cabina.getRadar().getReporte( values[i] ).getCivilizacion()!=this.civilizacion){
               return this.cabina.getControl().atacar(values[i]);             
                 }          
          }

  // attack asteroid         if (Espectro.ASTEROIDE == this.cabina.getRadar().getReporte( values[i] ).getEspectro()) {
                   return this.cabina.getControl().atacar(values[i]); 
            }

  // if found a container, upload materia 100        
         if (Espectro.CONTENEDOR == this.cabina.getRadar().getReporte( values[i] ).getEspectro()) {        
           while (this.cabina.getRadar().getReporte( values[i] ).getCantidadDeSustancia(Sustancia.ANTIMATERIA)>0){
              return this.cabina.getControl().transferirCarga(values[i], Direccion.ORIGEN, Sustancia.ANTIMATERIA, 100);           
              }           
         }

  // if found a ally base, download anti-materia 

         if (Espectro.BASE == this.cabina.getRadar().getReporte( values[i] ).getEspectro()) {          
           while (this.cabina.getMonitor().getCarga(Sustancia.ANTIMATERIA)>0){
              return this.cabina.getControl().transferirCarga(Direccion.ORIGEN,values[i], Sustancia.ANTIMATERIA, 100);            
              }            
         }

  // if there is nothing, move to a position     
        int ii = (int)Math.round(Math.random() *values.length-1) ;

        if (Espectro.VACIO == this.cabina.getRadar().getReporte( values[ii] ).getEspectro()){
            return this.cabina.getControl().avanzar( values[ii] );              
         }      
    //}

 // found none wait   
        return this.cabina.getControl().esperar();//.avanzar();

    }

    public Civilizacion getCivilizacion() {
        return this.civilizacion;
    }

    public String getNombre() {
        return "Piloto Imperial";
    }
}

Considering this is a framework and I only show you the class I'm changing, the rest works fine. This is a homework, so no need for a super fancy code. Last but not least, if you could help me use a pattern, that would be amazing.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ would you be so kind and rewrite your code in english please. and as far as i can see you make no difference between allied and hostile bases \$\endgroup\$ Jun 11 '13 at 14:26
  • \$\begingroup\$ you meant the objects? I cant, basically they are defined on external classes that i cannot touch... and notice on the allied based, there is not attack, basically it checks its position to know which spot is empty and then return to the origin. \$\endgroup\$ Jun 11 '13 at 14:37
  • 2
    \$\begingroup\$ i meant that the check whether there is an enemy or allied base is exactly the same. okay and to prepare my answer i need to know the types of this.cabina.getRadar().getReporte(values[i]).getEspectro() and this.cabina.getControl() \$\endgroup\$ Jun 11 '13 at 14:38
  • \$\begingroup\$ yeah, now that I see it, the compiler would never enter into the second one, thanks! \$\endgroup\$ Jun 11 '13 at 14:43
3
\$\begingroup\$

Your Professor has assigned you code that fails to meet some basic tenets of object-oriented programming, including:

Effectively, your Professor is teaching you how to program with structures, not objects, using Java.

For example, here's one problem with breaking the Law of Demeter:

if (Espectro.VACIO == this.cabina.getRadar().getReporte( values[ii] ).getEspectro()){

If any one of the following are null, the code will throw a NullPointerException:

  • this.cabina
  • getRadar()
  • getReporte( ... )

There are no try/catch blocks, nor does the method declare any exceptions, so the program will crash. A program that crashes can be quite frustrating for the users.

The line should be either of the following:

if( getCabina().isEspectro( Espectro.VACIO ) ) {

if( isEspectro( Espectro.VACIO ) ) {

In the second case, the method isEspectro would resemble:

public boolean isEspectro( Espectro e ) {
  return getCabina().isEspectro( e );
}

This is called delegation. It avoids the cascading dot notation that is the source of so many bugs. See this answer for more details about information hiding.

The method getCabina() would be wholly responsible for ensuring that the CabinaDeControl instance is set. Otherwise how do you guarantee that this.cabina is not null (without using a framework such as Spring, which supports inversion of control)? For example:

private synchronized CabinaDeControl getCabina() {
  if( this.cabina == null ) {
    this.cabinia = createCabina();
  }

  return this.cabina;
}

/** Subclasses can vary the CabinaDeControl instances used by this class. */
protected CabinaDeControl createCabina() {
  return new CabinaDeControl(); // ... or whatever is required to instantiate.
}

This is called lazy initialization. Importantly, the pattern follows the Open-Closed Principle whereby you can change the behaviour of a class by overriding the createCabina() method inside a subclass. You don't have to change the original class to change its behaviour. That prevents introducing widely-scoped bugs (the new subclass can still introduce bugs, but the ripple effect should be less severe than by changing the original class).

Using this.cabina.method() violates the DRY Principle because this.cabina is repeated several times. Programming means eliminating duplicate code and the reasons are many.

Each occurrence of this.cabina.method() can throw a NullPointerException because there is no check to ensure cabina is not null. That should leave you with an uneasy feeling. Replace this.cabina with getCabina() (as I have implemented above) and that uneasy feeling should go away. It does not mean the code will be correct, but at least the program won't crash if cabina is ever set to null.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The law of Demeter is to be taken with a grain of salt... Sometimes it goes too far. But otherwise, +1! \$\endgroup\$
    – fge
    Jun 11 '13 at 16:31
  • \$\begingroup\$ I´m very thankful for your help, I will take a look in deep. Thanks a ton \$\endgroup\$ Jun 11 '13 at 17:24
0
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ if you get used to this style i recommend another exercise for you: refactor CheckForEnemy and CheckForAlly into a single method that can be used to check for enemy, allied and neutral ships and bases ;) \$\endgroup\$ Jun 12 '13 at 13:04
  • \$\begingroup\$ Note that this code also breaks the principles I mentioned, in exactly the same ways. \$\endgroup\$ Jun 12 '13 at 16:05
  • \$\begingroup\$ @DaveJarvis what else except the null checks i left out? \$\endgroup\$ Jun 13 '13 at 14:40
  • \$\begingroup\$ Every internal reference to an internal variable must be hidden by an accessor call. So return orders.checkSituation(...) must be return getOrders().checkSituation(...). Otherwise the DRY Principle is violated, as would be the Open-Closed Principle for adding features in the future. \$\endgroup\$ Jun 13 '13 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.