0

I have an application, that contains a list of department names- when a user clicks one of those a JPanel showing its details should appear. I load a Department object in my model devoted class, but is it a bad practice to pass that entire object into the custom DepartmentDetailsPanel constructor and extract data to show from it there (such as name, List of User members etc...)?

public class DepartmentDetailsPanel extends JPanel {

    private ClassLoader cl = this.getClass().getClassLoader();
    private JPanel iconAndName = new JPanel();
    private JLabel icon;
    private JLabel name;

    private JPanel body = new JPanel();

    private JPanel basicInfo = new JPanel();
    private JLabel headLabel = new JLabel(Constants.head_name_lable);
    private JLabel managerLabel = new JLabel(Constants.manager_name_label);
    private JLabel typeLabel = new JLabel(Constants.type_label);

    private JLabel head;
    private JLabel manager;
    private JLabel type;

    private JPanel membersPanel = new JPanel();
    private JLabel usersLabel = new JLabel(Constants.members_label);
    private JList members;
    private JPanel memberButtons = new JPanel();
    private JButton removeButton = new JButton(Constants.remove);
    private JButton addButton = new JButton(Constants.add);

    public DepartmentDetailsPanel(Department d) {
        setLayout(new BorderLayout());
        setBorder(new EmptyBorder(10, 10, 10, 10) );

        iconAndName.setLayout(new FlowLayout(FlowLayout.LEFT));
        int dType = d.getType();
        if (dType == 1)
            icon = new JLabel(new ImageIcon(cl.getResource("leader.png")));
        if (dType == 2)
            icon = new JLabel(new ImageIcon(cl.getResource("creative.png")));
        if (dType == 3) 
            icon = new JLabel(new ImageIcon(cl.getResource("finance.png")));

        name = new JLabel(d.getName());
        iconAndName.add(icon);
        iconAndName.add(name);
        add(iconAndName, BorderLayout.NORTH);

        basicInfo.setLayout(new GridLayout(3, 2));
        basicInfo.add(typeLabel);
        type = new JLabel(Constants.department_types[dType-1]);
        basicInfo.add(type);
        basicInfo.add(headLabel);
        head = new JLabel(d.getHeadName());
        basicInfo.add(head);
        basicInfo.add(managerLabel);
        manager = new JLabel(d.getManagerName());
        basicInfo.add(manager);
        body.setLayout(new BoxLayout(body, BoxLayout.Y_AXIS));
        body.add(basicInfo);

        membersPanel.setLayout(new BorderLayout());
        membersPanel.add(usersLabel, BorderLayout.NORTH);
        JComponent memberList = makeList(extractMembers(d), members);
        membersPanel.add(memberList, BorderLayout.CENTER);
        memberButtons.setLayout(new BoxLayout(memberButtons, BoxLayout.Y_AXIS));
        memberButtons.add(addButton);
        memberButtons.add(removeButton);
        membersPanel.add(memberButtons, BorderLayout.EAST);
        body.add(membersPanel);
        add(body, BorderLayout.CENTER);
    }

    private JComponent makeList(String[] data, JList list) {
        JPanel panel = new JPanel();
        panel.setLayout(new GridLayout(1, 1));
        panel.setBackground(Constants.lightGrey);
        if (data == null){
            JLabel message = new JLabel(Constants.nothing_to_view);
            panel.add(message);
        } else {
            list = new JList(data);
            list.setVisibleRowCount(10);
            list.setSelectionMode(ListSelectionModel.SINGLE_SELECTION);
            list.setLayoutOrientation(JList.VERTICAL); 
            list.setBackground(Constants.lightGrey);
            panel.add(new JScrollPane(list));
        }
        return panel;
    }

    private String[] extractMembers(Department d){
        String[] members = null;
        List<User> list = d.getMembers();
        if (list != null || list.size()==0){
            members = new String[list.size()];
            int i = 0;
            for (User u: list){
                members[i] = u.getName() +" "+ u.getLastname();
                i++;
            }
        }
        return members;
    }

}
  • In my opinion it is a bad practice. A constructor should only be used to initialise your instance variables. If you need to do more (like in your case) you should think about creating a [factory method](http://stackoverflow.com/questions/929021/what-are-static-factory-methods-in-java). – Tom Apr 18 '15 at 10:44

2 Answers2

2

It's OK as long as you don't do anything except getting the field values. The view has to use the model, how else would it be possible to show the model?

Scadge
  • 9,380
  • 3
  • 30
  • 39
0

You should pay attention to the difference between Objects and data structures. A data structure is an instance of a class that has only getters and setters. Objects are more intelligent because they do some calculation or some processing. find more here http://blog.8thlight.com/uncle-bob/2013/10/01/Dance-You-Imps.html

MVC has become one of the most confusing patterns so far, because it doesn't serve any more the purpose it was originally created for.

The view layer in MVC pattern has basically two roles : representing data, and receiving user actions. Data representation implies keeping somehow view state and model state. You can for example do this by passing the model data to the view even if I don't like this approach. But there are other mechanisms that you can use. For example you can use binding techniques that update data state both on the view and model. But The most important thing to do is to keep one way to do data representation in all your application especially if you're working with a team. You can read more about GUI Architectures here : http://martinfowler.com/eaaDev/uiArchs.html