Menu Model class using Model-View-Presenter

Question

The following code is for my Object Oriented Programming class. My program simulate the cashier's machine to order food. The code uses Model View Presenter. This class is the model.

I have an ArrayList to hold the possible order possible. The View uses it to create JButtons.

I have an ArrayList to hold data to know what is ordered.

I am wondering if I followed Model-View-Presenter correctly.
Is this class holding too much responsibility.

public class MenuModel {
    // ArrayList hold MenuItem
    private ArrayList<MenuItem> menuItems;
    // Textfile name
    private String fileName = "Menu.txt"; 
    // Use to read textfile 
    private MenuReader menuReader;
    // Data to keep menu's price and the menu's order
    //Keep for what was ordered
    private ArrayList<MenuItem> order;
    //Listeners
    private ArrayList<ChangeListener> listeners;
    private Formatter formatter;

    /**
     * 
     * @param file name 
     */
    public MenuModel(String f){
        listeners = new ArrayList<ChangeListener>();
        formatter = new Formatter();
        order = new ArrayList<MenuItem>();
        fileName = f;
        menuItems = new ArrayList<MenuItem>();
        menuReader = new MenuReader(fileName, '|', menuItems);
        menuReader.read(); 
    }

    public void addChangeListener(ChangeListener listener){
        listeners.add(listener);
    }
    // update the menu & price
    public void add(MenuItem menuItem){
        order.add(menuItem); 
        ChangeEvent event = new ChangeEvent(this);
        for(ChangeListener lister : listeners)
            lister.stateChanged(event);
    }
     /**
      * 
      * @return Menu Order Size
      */
    public int size(){
        return menuOrder.size();
    }

    // clear price and menu
    public void clear(){        

        order.clear(); 
        ChangeEvent event = new ChangeEvent(this);
        for(ChangeListener lister : listeners)
            lister.stateChanged(event);

    }
    // Text to be sent to the screen
    public String display(){    
        StringBuilder sb = new StringBuilder();
        sb.append(formatter.formatHeader());
        Iterator<MenuItem> iter = getItems();
        while(iter.hasNext())
            sb.append(formatter.formatOrder(iter.next()));

        sb.append(formatter.formatFooter());
        return sb.toString();  
    }
    // Possible order available 
    public ArrayList<MenuItem> getMenuItem(){ return menuItems; }
    // Get what was ordered
    public ArrayList<MenuItem> getOrderList(){ return order; }
    /**
     * 
     * @return String filename
     */
    public String getFileName(){ return fileName; }

    public Iterator<MenuItem> getItems(){
        return new Iterator<MenuItem>(){
            private int current = 0;
            @Override
            public boolean hasNext(){
                return current < order.size();
            } 
            @Override
            public MenuItem next() {
                 return order.get(current++);
            }
            @Override
            public void remove() {
                throw new UnsupportedOperationException();
            }
        };
    }
}

Answer 1

I would separate the responsibility to load the file from this class.I think this class have more responsibilities of a Presenter than a model class. A good indicator of that is the method display and the method clear and seems to me that you are using the Observer pattern to inform the view that the model has changed, and this seems to be the work of a middle-man class (Presenter).

So the only 2 thing I would change is to create a class to make the load of the file, and I would use for-each instead iterator, seems to me that iterator is something used much more in C++ than in Java.

I hope this can help :)

Answer 2

Interfaces over implementations

// Possible order available 
public ArrayList<MenuItem> getMenuItem(){ return menuItems; }
// Get what was ordered
public ArrayList<MenuItem> getOrderList(){ return order; }

These methods should return a List, not an ArrayList. This is because method callers should only know that they are dealing with a List implementation, instead of an ArrayList specifically.

On a related note, you may want to reformat these lines to follow the standard Java convention, as you have done so almost consistently elsewhere (hint: your loop constructs are missing curly brackets).

Superfluous methods

I have no idea why you need to roll your own Iterator implementation for getItems(): wouldn't returning order.iterator() be good enough?

Java 8 String concatenation alternative

If you happen to be on Java 8, the display() implementation can be fluently replaced with a stream-based approach:

public String display() {
    return getOrderList().stream()
                    .map(formatter::formatOrder) // assuming this returns a String
                    .collect(Collectors.joining("", formatter.formatHeader(), 
                                                    formatter.formatFooter()));
}

Here, we use formatter.formatOrder(MenuItem) as a method reference, and join the elements together with the provided header and footer (assuming these are returned as Strings too).