"Agenda" test application for Enum

Question

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?

Answer 1

Just a few random comments:

1. For this:

   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?

   You might not want this to be calculated automatically. A maintainer might insert a new enum value in the middle of the other values which would break the existing "hotkeys": code for the listing items menu was 3 but in a new release it's changed to 4. It could be frustrating to users.

2. Mixing two languages in the code makes it harder to read and maintain. Maintainers have to remember which method/variable is in English and which one is in Italian. Using only one language requires less work.

3. For this:

   Well, I don't like it really, it sounds like a strange solution. How could it be replaced?

   Another idea is creating a separate class for every Action and storing the available instances in a list/map.

   public interface Action {
       String getKeyCode();
       void doAction() throws MenuExitException;
   }
   

   Usage:

   List<Action> actions = new Action<>();
   actions.add(new OpenAction());
   actions.add(new ExitAction());
   
   for (Action action: actions) {
       if (!action.getKeyCode().eqauls(pressedKey)) {
           continue;
       }
       action.doAction();
       return;
   }
   // invalid action
   

   Note the MenuExitException too (instead of the boolean return value). It's another way of exitting the application.

4. The following pattern is in the code multiple times:

   if (nome == null)
       throw new IllegalArgumentException("nome == null");
   

   I'd use Guava's checkArgument method here (or create a similar one). With checkNotNull you can save another line (although it throws NullPointerException instad of IllegalArgumentException):

   this.nome = checkNotNull(nome, "nome == null");
   

5. You could use Objects.hash in the hashCode method.

6. This assignment could be in the same line as the declaration:

   private Set<Contatto> contatti;
   
   public Agenda()
   {
       contatti = new HashSet<Contatto>();
   }
   

   And it could be final:

   private final Set<Contatto> contatti = new HashSet<Contatto>();
   

   final would improve code readability since readers don't have to check whether it's is changed somewhere in the class or not.

7. I would not restrict the phone number to have only numbers.

   If the user wants to give you his phone number, then trust him to get it right. If he does not want to give it to you then forcing him to enter a valid number will either send him to a competitor's site or make him enter a random string that fits your regex. I might even be tempted to look up the number of a premium rate sex line and enter that instead.

   Source: https://stackoverflow.com/a/1245990/843804

8. Anyway, a named method (readPhoneNumber()) for the phone number reading logic would be easier to follow here.

9. Instead of ignoring IOException log them. It could help debugging a lot. (Having "does not work" bug reports without any possible/available clue about what went wrong could harm you. Saying that you have no idea is not too professional.)

10. Instead of

    if (contatti.size() == 0)
    

    the following would be more convenient:

    if (contatti.isEmpty())
    

11. Furthermore, instead of this:

    if (contatti.isEmpty())
    {
        System.out.println("Nessun contatto.");
    }
    else
    {
        for (Contatto contatto : contatti)
        {
            System.out.println(contatto);
        }
    }
    

    you could use a guard clause to make the code flatten:

    if (contatti.isEmpty()) {
        System.out.println("Nessun contatto.");
        return;
    }
    
    for (final Contatto contatto: contatti) {
        System.out.println(contatto);
    }
    

12. MENU_SCELTE.from refers to the value as code. It would be easier to read if both were called menuCode or something more descriptive.

13. chooseOption() also could use a guard clause:

    private static MENU_SCELTE chooseOption() {
        while (true) {
            try {
                int userScelta = Integer.parseInt(Actions.bufferedReader.readLine());
                return MENU_SCELTE.from(userScelta);
            } catch (IOException ignored) {
            } catch (NumberFormatException e) {
                System.out.println("Inserisci un numero valido.");
            }
        }
    }
    

14. Referring to Actions.bufferedReader from Main seems too tightly coupled:

    userScelta = Integer.parseInt(Actions.bufferedReader.readLine());
    

    I would create it only once and pass it to the object instances which require it. It would also make testing easier (you can give them a mocked/fake reader) and make dependencies visible which is easier to work with (no surprises, you won't find a static method call/field access in the middle of another method, it will be clear which class requires which one at the beginning).

    I would also rename bufferedReader to something which reflects that it's reading from the stdin.