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();
 }
 };
 }
}

• user102889: SOMEONE HELP ME!

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

• Kim Aragon Escobar: I'm not sure if I understood the "more abstraction". If you are talking about create a class that make the load, yes I guess you need it. I really think that the MenuModel seems to be a Presenter so I would change the name of the class to ManuPresenter, and I don't really know if worth it to have a MenuModel, I think the presenter is enough. :)
• user102889: No the intend of MenuModel is store the data. My professor provided a template. I am just filling in the skeleton code she provided. I have a separate Presenter class.
• Kim Aragon Escobar: hmmm, in that case if is possible I would take of the display method and the listener and event change from the menu, seems to me that this responsibilities would be from a presenter class not a model class. As aforementioned I would to create a class that reads and provides the list of menuitems, does not seems to be a responsibility of the menuitem doing so.
• user102889: Make my "MenuReader" class provide an ArrayList<MenuItem> ?
• Kim Aragon Escobar: Sorry I misread the code, but I would do the MenuReader::read return the list instead of passing the list to filled by reference. But this is beyond the point of this post .

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