5
\$\begingroup\$

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();
            }
        };
    }
}
\$\endgroup\$
  • \$\begingroup\$ SOMEONE HELP ME! \$\endgroup\$ – None Apr 14 '16 at 18:15
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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. :) \$\endgroup\$ – Kim Aragon Escobar Apr 14 '16 at 19:34
  • \$\begingroup\$ 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. \$\endgroup\$ – None Apr 14 '16 at 19:38
  • \$\begingroup\$ 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. \$\endgroup\$ – Kim Aragon Escobar Apr 14 '16 at 19:42
  • \$\begingroup\$ Make my "MenuReader" class provide an ArrayList<MenuItem> ? \$\endgroup\$ – None Apr 14 '16 at 19:48
  • \$\begingroup\$ 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 . \$\endgroup\$ – Kim Aragon Escobar Apr 14 '16 at 19:57
2
\$\begingroup\$

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

\$\endgroup\$

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.