1
\$\begingroup\$

I had to create a selection menu for a Contact Directory project for school. This is the second time I've had to create a menu for a particular project so I recycled the menu from my previous project to accommodate this Contact Directory and all went well.

But I realized I'm basically just listing the Objects methods for the user to select from in both scenarios so I got bored and decided to try to generalize the menu class I had to work with any directory-type class and this is what I came up with.

package menu2;

import java.lang.reflect.Method;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;

import javax.swing.JOptionPane;

public class Menu2 {

    /**
     * @param args the command line arguments
     * @throws java.io.IOException
     */
    private String name;
    private int selection;
    private int count;
    private Class c;
    private String menuDisplay = " ";
    private Method[] methods; 
    /**
     *creates menu to navigate directory list
     * @throws IOException
     */ 

    public Menu2(Object o) throws IOException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
        this.c = o.getClass();
        menuView();
        selection  = 0;
        do {
            selection = menu();
            if(selection != count) {
                methods[selection-1].invoke(o);
            }
        } while (selection != count);
    } 

    /**
     *displays menu options
     * @return
     */
    public void menuView() {
        methods = c.getDeclaredMethods();
        Method[] temp = new Method[methods.length];
        count = 0;
        for (int i = 0; i < methods.length; i++) {
                System.out.println("public method: " + c.getSimpleName());
                if(methods[i].getParameterCount()== 0 && !methods[i].getName().equalsIgnoreCase("iterator")&&!methods[i].getName().equalsIgnoreCase("tostring")){
                    menuDisplay += (count+1)+". "+methods[i].getName()+"\n";
                    temp[count] = methods[i];
                    count++;
                }
        }
        menuDisplay += count+". Quit";
        methods = temp;

        /*Method[] x = new Method[count];
        for(int i = 0; i < count;i++){
            x[i] = temp[i];
        }
        methods = x;*/
    }

    public int menu() {
        String choiceStr;
        int choice;   
        choiceStr = JOptionPane.showInputDialog(menuDisplay);
        choice = Integer.parseInt(choiceStr);
        return choice;
    }
}

I'm fairly new to programming so I'm just looking for insight as to how to improve my code or if there's a better way to do this.

Driver:

package driver;
import contactdir.ContactDir;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;

public class Driver {
    public static void main(String[] args) throws IOException, IllegalAccessException, InvocationTargetException{
        ContactDir dir = new ContactDir();
        //Menu dirMenu = new Menu();     
    } 
}

Directory:

package contactdir;

import contactinfo.ContactInfo;

import javax.swing.JOptionPane;
import jsjf.*;
import fileman.FileMan;
import java.awt.Dimension;
import java.io.*;
import java.lang.reflect.InvocationTargetException;
import java.util.*;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import menu2.Menu2;

public class ContactDir implements Iterable<ContactInfo>, Serializable {

    private ArrayOrderedList<ContactInfo> list;
    private FileMan<ContactInfo> fman; 
    private JScrollPane scroll;
    private JTextArea text;
    private boolean value = false;

    public ContactDir() throws IOException, IllegalAccessException, IllegalArgumentException, InvocationTargetException{
        this.list = new ArrayOrderedList<>();
        this.fman = new FileMan<>();
        Menu2 menu = new Menu2(this);
    }    
    public void addContact(){
         list.add(new ContactInfo());

    }    
    public void addContact(ContactInfo contact){
        if(contact != null && list != null){

            list.add(contact);
        }
    }
   public void deleteContact(ContactInfo contact){
        if(contact != null && list != null)
            list.remove(contact);
    }
    /**
     *deletes contact from list
     * @param name
     * @return
     */
    public boolean deleteContact(String name){
        boolean value = false;
        if(list != null){
            deleteContact(findContact(name));
            value = true;
        }
        return value;
    }
    public boolean deleteContact(){
        String name = JOptionPane.showInputDialog("Enter the name of the contact to remove:");
        boolean value = false;
        if(list != null){
            deleteContact(findContact(name));
            value = true;
        }
        return value;
    }

    /**
     *finds contact in list
     * @param name
     * @return
     */
    public ContactInfo findContact(String name){
        for(ContactInfo info : list ){
            if(info.getName().equalsIgnoreCase(name))
                return info;  
        }
        return null;
    }
    /**
     *updates existing contact in list
     * @param name
     * @return
     */
    public boolean updateContact(String name){

        value = false;
        ContactInfo temp = findContact(name);
        String choice = "Update Name? Update Address? Update Email? Update Work Number? Update Cell Number?";
        Scanner scan = new Scanner(choice);
        scan.useDelimiter("\\?");
        String par = "";
        int confirm = 1;

        while(temp != null && confirm != JOptionPane.YES_OPTION){
            par = scan.next();
            System.out.println("par : "+par);
            confirm = JOptionPane.showConfirmDialog(null,par+"?");
        }
        par = par.trim();

        if(par.equalsIgnoreCase("update name")){
            JOptionPane.showMessageDialog(null, "Enter the name of the contact...");
            String n = JOptionPane.showInputDialog("name: ");
            temp.setName(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("Update Address")){
            JOptionPane.showMessageDialog(null, "Enter the address of the contact...");
            String n = JOptionPane.showInputDialog("address: ");
            temp.setAddress(n);
            value = true;
        }
        else if (par.equalsIgnoreCase("update email")){
            JOptionPane.showMessageDialog(null, "Enter the email of the contact...");
            String n = JOptionPane.showInputDialog("email: ");
            temp.setEmail(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("update work number")){
            JOptionPane.showMessageDialog(null, "Enter the work number of the contact...");
            String n = JOptionPane.showInputDialog("number: ");
            temp.setWorknum(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("update cell number")){
            JOptionPane.showMessageDialog(null, "Enter the cell number of the contact...");
            String n = JOptionPane.showInputDialog("number: ");
            temp.setCellnum(n);
            value = true;
        }
        else
            System.out.println("Error");
        return value;        
    }
    public boolean updateContact(){

        value = false;
        String name = JOptionPane.showInputDialog("Enter the name of the contact to update:");
        ContactInfo temp = findContact(name);
        String choice = "Update Name? Update Address? Update Email? Update Work Number? Update Cell Number?";
        Scanner scan = new Scanner(choice);
        scan.useDelimiter("\\?");
        String par = "";
        int confirm = 1;

        while(temp != null && confirm != JOptionPane.YES_OPTION){
            par = scan.next();
            System.out.println("par : "+par);
            confirm = JOptionPane.showConfirmDialog(null,par+"?");
        }
        par = par.trim();

        if(par.equalsIgnoreCase("update name")){
            JOptionPane.showMessageDialog(null, "Enter the name of the contact...");
            String n = JOptionPane.showInputDialog("name: ");
            temp.setName(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("Update Address")){
            JOptionPane.showMessageDialog(null, "Enter the address of the contact...");
            String n = JOptionPane.showInputDialog("address: ");
            temp.setAddress(n);
            value = true;
        }
        else if (par.equalsIgnoreCase("update email")){
            JOptionPane.showMessageDialog(null, "Enter the email of the contact...");
            String n = JOptionPane.showInputDialog("email: ");
            temp.setEmail(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("update work number")){
            JOptionPane.showMessageDialog(null, "Enter the work number of the contact...");
            String n = JOptionPane.showInputDialog("number: ");
            temp.setWorknum(n);
            value = true;
        }
        else if(par.equalsIgnoreCase("update cell number")){
            JOptionPane.showMessageDialog(null, "Enter the cell number of the contact...");
            String n = JOptionPane.showInputDialog("number: ");
            temp.setCellnum(n);
            value = true;
        }
        else
            System.out.println("Error");
        return value;        
    }

    /**
     *displays contact in list
     * @param name
     * @return
     */
    public boolean displayContact(){
        String name = JOptionPane.showInputDialog("Enter the name of the contact to find:");
        if(list != null){
            JOptionPane.showMessageDialog(null,findContact(name).toString());
            value = true;
        }
        else
            JOptionPane.showMessageDialog(null,"Object not found");
        return value;
    }

    /**
     *displays all contacts in list
     */
    public void displayDir(){

        text = new JTextArea((this.toString()));
        text.setLineWrap(true);  
        text.setWrapStyleWord(true); 
        scroll = new JScrollPane(text);
        scroll.setPreferredSize( new Dimension( 500, 500 ) );
        if(list != null)  
            JOptionPane.showMessageDialog(null, scroll);
    }

    @Override
    public String toString() {
        return "Contact Directory:\n" + list;
    }

    /**
     *saves list to file
     * @return
     * @throws IOException
     */
    public boolean save() throws IOException{

        fman.save("Directory.txt", list);
        return value;
    }

    /**
     *loads list from file
     * @return
     * @throws IOException
     */
    public boolean load() throws IOException{
        list = fman.load("Directory.txt");  
        return value;
    }

    @Override
    public Iterator<ContactInfo> iterator() {
        return list.iterator();//To change body of generated methods, choose Tools | Templates.
    }

    /**
     * @param args the command line arguments
     */


}
\$\endgroup\$
1
\$\begingroup\$

You might want to turn on automatic code formatting in your editor. In Eclipse this can be found under Window>Preferences>Java>Editor>Save actions. Consistent formatting will make your code more readable for you and other specially people.

There's also an unused variable in the code. A Java IDE should be able to highlight problems like that, so you can easily fix them. Unused code shouldn't be kept around as it's just confusing to the reader.

It's a bit unusual to use meta programming in normal application code but I think it's great that you learn and explore these options. You find many discussions on the problems with reflections and ways to avoid it on this site, eg., here and here.

Your methods and variables sometimes could have more descriptive names. For example count is used as a counter to set the indices in the UI text but is also used in the exit condition of the while loop. This way you have to read menuView() to understand the while loop.

I've reworked the code a bit to use the stream api and added a comment here and there:

public class Menu2 {

    // deleted comment that didn't match anything
    // deleted unused variable
    private int selection; // ints are always implicitly initialized with 0
    private String menuDisplay;
    private List<Method> callableMethods;
    private int quitOptionIndex;

    /**
     * creates menu to navigate directory list
     * 
     */
    public Menu2(Object o) throws IllegalAccessException, IllegalArgumentException, InvocationTargetException {
        callableMethods = findCallableMethods(o.getClass());
        menuDisplay = buildMenuMessage();

        do {
            selection = showMenuView();
            if (selection > 0 && selection < quitOptionIndex) { // guard against IndexOutOfBoundException
                callableMethods.get(selection - 1).invoke(o);
            }
        } while (selection != quitOptionIndex);
    }

    private List<Method> findCallableMethods(Class<? extends Object> klass) {
        return Arrays.stream(klass.getDeclaredMethods()) // convert array to stream
                .filter(method -> method.getParameterCount() == 0 && !method.getName().equalsIgnoreCase("iterator")
                        && !method.getName().equalsIgnoreCase("tostring")) // only select methods the user can call
                .collect(Collectors.toList()); // collect to list
    }

    private String buildMenuMessage() {
        StringBuilder sb = new StringBuilder(); // using string concatination in a loop might give you poor performance, since the list will never be that long it's probably overkill here
        for (int i = 0; i < callableMethods.size(); i++) {
            sb = sb.append(i + 1).append(". ").append(callableMethods.get(i).getName()).append(System.lineSeparator());
        }
        quitOptionIndex = callableMethods.size() + 1;
        return sb.append(quitOptionIndex + ". " + "Quit").toString();
    }

    public int showMenuView() {
        String choice = JOptionPane.showInputDialog(menuDisplay);
        return Integer.parseInt(choice);
    }

}

If it's okay for you to have the user pick from a dropdown then you could get rid of the index by having the UI selection options backed by an array of methods:

public class Menu2 {

    class SelectableMethod {

        private Method method;

        public SelectableMethod(Method method) {
            this.method = method;
        }

        public void invoke(Object obj)
                throws IllegalAccessException, IllegalArgumentException, InvocationTargetException {
            method.invoke(obj);
        }

        // JOptionPane calls this to get the strings to display in the dropdown
        @Override
        public String toString() {
            return method.getName();
        }

    }

    private List<SelectableMethod> callableMethods;

    /**
     * creates menu to navigate directory list
     * 
     */
    public Menu2(Object o) throws IllegalAccessException, IllegalArgumentException, InvocationTargetException {
        callableMethods = findCallableMethods(o.getClass());

        while (true) {
            showMenuView().invoke(o);
        }
    }

    private List<SelectableMethod> findCallableMethods(Class<? extends Object> klass) {
        return Arrays.stream(klass.getDeclaredMethods()) // convert array to stream
                .filter(method -> method.getParameterCount() == 0 && !method.getName().equalsIgnoreCase("iterator")
                        && !method.getName().equalsIgnoreCase("tostring")) // only select methods the user can call
                .map(SelectableMethod::new)
                .collect(Collectors.toList()); // collect to list
    }

    public SelectableMethod showMenuView() {
        // since the selectable items are backed by the Methods we don't need the index anymore
        return (SelectableMethod) JOptionPane.showInputDialog(null, "Choose a method", "Choose a method",
                JOptionPane.QUESTION_MESSAGE, null,
                callableMethods.toArray(new Object[callableMethods.size()]), 0);
    }

}

Edit: I sadly don't have any more time right now but have a look at this implementation of the strategy pattern. Instead of implementing the methods that your user should be able to call on the Dir, save them in a Map or List inside the Dir, as lambdas (anonymous functions). Then Menu2 can then get them like it currently does with klass.getDeclaredMethods() by iterating over the values of the collection.

Hope this helps and sends you in the right direction.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for the feedback I really appreciate it, I’m going to go over your edits so I can learn more they are great \$\endgroup\$ – bl00dbath Nov 11 '18 at 1:09
  • 1
    \$\begingroup\$ Welcome to Code Review! Can you please expand on the specific improvements you made to the author's code, so they (and others) can more easily understand how it's better. \$\endgroup\$ – Quill Nov 11 '18 at 1:26
  • \$\begingroup\$ So just for clarification reflection is bad to use like that? Is there a better way to implement menu? The edits are great but they still use reflection which isn’t ideal correct? \$\endgroup\$ – bl00dbath Nov 11 '18 at 2:14
  • \$\begingroup\$ @Quill I tried to explain changes in my code comments. Do you think I should explain more? \$\endgroup\$ – fap Nov 11 '18 at 2:17
  • \$\begingroup\$ @bl00dbath Can you please share some code that is calling Menu2 / give some more context (you can edit your original question). This would make answering this question easier. There are definitely good use cases for reflection but you should be aware of the trade-offs. \$\endgroup\$ – fap Nov 11 '18 at 2:26

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.