I'm still new in Java so I always try to find new ways and improve my skills.
I found enum
in Java to be very powerful (maybe excessively?) and while I was reading Effective Java book I've had some great ideas and I thought about using it to improve the flexibility of a standard console application which asks to the user to select 1...x options and does actions with it (to avoid the problems to update indexes options every time you need to add/remove an option).
Ok, to test it I've just created a small application which doesn't do anything special (add and remove elements from a Set
in logic code)
The application allows the user to add a contact, remove it and show all contacts.
Main.java
The main class of the application, show the menu and ask to the user to select an option and execute the selected option.
Code:
public class Main
{
public static void main(String[] args)
{
Agenda agenda = new Agenda();
boolean result = true;
while (result)
{
for (MENU_SCELTE scelte : MENU_SCELTE.values())
{
System.out.println(scelte);
}
MENU_SCELTE scelta = chooseOption();
if (scelta == null)
{
System.out.println("Scelta non valida.");
}
else
{
result = scelta.perform(agenda);
}
}
}
/**
* This method will create the menu
*
* @return True if the program should continue to create the menu; false otherwise.
*/
private static MENU_SCELTE chooseOption()
{
int userScelta;
while (true)
{
try
{
userScelta = Integer.parseInt(Actions.bufferedReader.readLine());
break;
}
catch (IOException ignored) { }
catch (NumberFormatException e)
{
System.out.println("Inserisci un numero valido.");
}
}
return MENU_SCELTE.from(userScelta);
}
}
MENU_SCELTE.java
MenuAction
is an interface which declares only one method (perform
) which executes the method.
While it would be better to define it inside the MENU_SCELTE
as abstract method
I chose to use an interface for no reason... Maybe an interface would be better in a more serious scenario.
A description of the action is passed to the constructor; the value (the number to write to execute the action) is calculated using ordinal() + 1
it could be read as a bad practice but I may be wrong, am I?
public enum MENU_SCELTE implements MenuAction
{
AGGIUNGI_CONTATTO ("Permette di aggiungere un contatto")
{
@Override
public boolean perform(Agenda agenda)
{
Actions.aggiungiContatto(agenda);
return true;
}
},
RIMUOVI_CONTATTO ("Permette di rimuovere un contatto")
{
@Override
public boolean perform(Agenda agenda)
{
Actions.rimuoviContatto(agenda);
return true;
}
},
ELENCO_CONTATTI ("Elenca tutti i contatti")
{
@Override
public boolean perform(Agenda agenda)
{
Actions.elencoContatti(agenda);
return true;
}
},
ESCI ("Chiude il programma")
{
@Override
public boolean perform(Agenda agenda)
{
return false;
}
};
private static final EnumMap<MENU_SCELTE, Integer> map;
private final int value;
static
{
map = new EnumMap<MENU_SCELTE, Integer>(MENU_SCELTE.class);
for (MENU_SCELTE scelte : values())
{
map.put(scelte, scelte.value);
}
}
private final String description;
MENU_SCELTE(String description)
{
this.description = description;
value = ordinal() + 1;
}
@Override
public String toString()
{
return value + ": " + description;
}
public static MENU_SCELTE from(int code)
{
for (EnumMap.Entry<MENU_SCELTE, Integer> entry : map.entrySet())
{
if (entry.getValue() == code)
{
return entry.getKey();
}
}
return null;
}
}
MenuAction.java
I know the Javadoc is wrong.
public interface MenuAction
{
/**
* Will be the action which should be executed when selected
*
* @return Returns true if the application can show again the menu; false if not.
*/
boolean perform(Agenda agenda);
}
Actions.java
Here is my main problem, the code inside the enum
will start to be very big if I implement the code of the various actions inside the perform method so I created some static methods which contain the logic.
Well, I don't like it really, it sounds like a strange solution. How could it be replaced?
To avoid creating a new BufferedReader
every time I used a static one.
public class Actions
{
public static final BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(System.in));
public static void aggiungiContatto(Agenda agenda)
{
try
{
Contatto.Builder builder = new Contatto.Builder();
System.out.println("Inserisci il nome del contatto");
// i don't care if he uses a bad name
builder.setNome(bufferedReader.readLine());
System.out.println("Inserisci il cognome del contatto.");
builder.setCognome(bufferedReader.readLine());
String phoneNumber;
while (true)
{
try
{
System.out.println("Inserisci il numero di telefono");
phoneNumber = bufferedReader.readLine();
// 20 - but could be 15?
if (phoneNumber.length() < 20 && tryIsNumeric(phoneNumber))
{
break;
}
else
{
System.out.println("Inserisci un numero valido.");
}
}
catch (IOException ignored) { }
}
builder.setPhoneNumber(phoneNumber);
if (agenda.add(builder.build()))
{
System.out.println("Contatto aggiunto!");
}
else
{
System.out.println("Contatto non aggiunto, già presente.");
}
}
catch (IOException e)
{
// e.printStackTrace();
}
}
public static void rimuoviContatto(Agenda agenda)
{
try
{
System.out.println("Nome contatto:");
String nome = bufferedReader.readLine();
System.out.println("Cognome contatto:");
String cognome = bufferedReader.readLine();
System.out.println("Numero di telefono:");
String phoneNumber = bufferedReader.readLine();
if (agenda.remove(nome, cognome, phoneNumber))
{
System.out.println("Contatto rimosso.");
}
else
{
System.out.println("Impossibile rimuovere il contatto.");
}
}
catch (IOException e)
{
e.printStackTrace();
}
}
public static void elencoContatti(Agenda agenda)
{
Set<Contatto> contatti = agenda.getContatti();
if (contatti.size() == 0)
{
System.out.println("Nessun contatto.");
}
else
{
for (Contatto contatto : contatti)
{
System.out.println(contatto);
}
}
}
/**
* Check if a String is a Number but doesn't convert it.
*
* It works checking if Long.parseLong thrown an NumberFormatException
* @param string The string to check
* @return True if the string contains only numbers; otherwise false.
*/
public static boolean tryIsNumeric(String string)
{
try
{
Long.parseLong(string);
return true;
}
catch (NumberFormatException e) { return false; }
}
}
Agenda.java
This class is nothing special, it just keeps a List of Contatto
.
What about HashSet
as collection? And the remove(String, String, String)
method? Maybe I could use a foreach
, since I will stop the loop after I've deleted it, so no ConcurrentModificationException
should be thrown?
public class Agenda
{
private Set<Contatto> contatti;
public Agenda()
{
contatti = new HashSet<Contatto>();
}
public boolean add(Contatto contatto)
{
return contatti.add(contatto);
}
public void remove(Contatto contatto)
{
contatti.remove(contatto);
}
public boolean remove(String nome, String cognome, String phoneNumber)
{
Iterator<Contatto> iterator = contatti.iterator();
while (iterator.hasNext())
{
Contatto contatto = iterator.next();
if (contatto.getNome().equals(nome) && contatto.getCognome().equals(cognome) && contatto.getPhoneNumber().equals(phoneNumber))
{
iterator.remove();
return true;
}
}
return false;
}
public Set<Contatto> getContatti()
{
return Collections.unmodifiableSet(contatti);
}
}
Contatto.java
While i know hashCode
method do useless checks (nome
, cognome
and phoneNumber
will never be null so why check it?)
It's a basic class.
public final class Contatto
{
public static class Builder
{
private String nome;
private String cognome;
private String phoneNumber;
public Builder setNome(String nome)
{
if (nome == null)
throw new IllegalArgumentException("nome == null");
this.nome = nome;
return this;
}
public Builder setCognome(String cognome)
{
if (cognome == null)
throw new IllegalArgumentException("cognome == null");
this.cognome = cognome;
return this;
}
public Builder setPhoneNumber(String phoneNumber)
{
if (phoneNumber == null)
throw new IllegalArgumentException("phoneNumber == null");
this.phoneNumber = phoneNumber;
return this;
}
public Contatto build()
{
return new Contatto(this);
}
}
private final String nome;
private final String cognome;
private final String phoneNumber;
private Contatto(Builder builder)
{
nome = builder.nome;
cognome = builder.cognome;
phoneNumber = builder.phoneNumber;
}
public String getNome()
{
return nome;
}
public String getCognome()
{
return cognome;
}
public String getPhoneNumber()
{
return phoneNumber;
}
@Override
public boolean equals(Object obj)
{
if (obj instanceof Contatto)
{
Contatto contatto = (Contatto) obj;
return
contatto.phoneNumber.equals(phoneNumber) &&
contatto.nome.equals(nome) &&
contatto.cognome.equals(cognome);
}
return false;
}
@Override
public int hashCode()
{
int result = nome != null ? nome.hashCode() : 0;
result = 31 * result + (cognome != null ? cognome.hashCode() : 0);
result = 31 * result + (phoneNumber != null ? phoneNumber.hashCode() : 0);
return result;
}
@Override
public String toString()
{
return nome + " " + cognome + " (" + phoneNumber + ")";
}
}
The application works very well, but its design is terrible for me... how could it be improved?