Battleship type game in Java

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 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

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

• Information Hiding

• The Law of Demeter

• The Open-Closed Principle

• The DRY Princple

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.

StackExchange Code Review Q#27268, answer score: 3