0
\$\begingroup\$

I'm designing a simple GUI using tabs to separate each 'window' of the application. There are 3 main tabs, each with 2 sub-tabs.

In one class I create the entire hierarchy of panes and panels:

mainPane = new JTabbedPane();
mainPane.setBounds(0, 0, WIDTH, HEIGHT);

//instantiate main tabs, get panes added to them
createTab = new JPanel();
createTab.setLayout(null);

customizeTab = new JPanel();
customizeTab.setLayout(null);

viewTab = new JPanel();
viewTab.setLayout(null);

//instantiate main panes, get sub-tabs added to them
createPane = new JTabbedPane();
createPane.setBounds(0, 0, WIDTH, HEIGHT);

customizePane = new JTabbedPane();
customizePane.setBounds(0, 0, WIDTH, HEIGHT);

viewPane = new JTabbedPane();
viewPane.setBounds(0, 0, WIDTH, HEIGHT);

//instantiate sub-tabs, get elements added to them
createEntityTab = new JPanel();
createEntityTab.setLayout(null);

createDealTab = new JPanel();
createDealTab.setLayout(null);

customizeEntityTab = new JPanel();
customizeEntityTab.setLayout(null);

customizeDealTab = new JPanel();
customizeDealTab.setLayout(null);

viewEntityTab = new JPanel();
viewEntityTab.setLayout(null);

viewDealTab = new JPanel();
viewDealTab.setLayout(null);

//add sub-tabs to main panes
createPane.addTab("Create Entity",null, createEntityTab);
createPane.addTab("Create Deal", null, createDealTab);

customizePane.addTab("Customize Entity", null, customizeEntityTab);
customizePane.addTab("Customize Deal", null, customizeDealTab);

viewPane.addTab("View Entity", null, viewEntityTab);
viewPane.addTab("View Deal", null, viewDealTab);

//add main panes to main tabs
createTab.add(createPane);
customizeTab.add(customizePane);
viewTab.add(viewPane);

//add main tabs to mane pane
mainPane.addTab("Create", null, createTab, "");
mainPane.addTab("Customize", null, customizeTab, "");
mainPane.addTab("View", null, viewTab, "");

and now to add elements to each sub-tab, I would like to do something like this:

new CreateEntity(createEntityTab);
new CreateDeal(createDealTab);

i.e., passing the "sub-tab" and then adding elements in the CreateEntity class.

This seems to work, but my question is the following:

Is using a constructor to pass a JPanel, and then adding elements in the new class considered bad coding?

I know using null in layout is bad practice. It's just for the question.

\$\endgroup\$
  • \$\begingroup\$ Your question would be clearer if you could provide the entire CreateEntity class. \$\endgroup\$ – Simon Forsberg May 25 '14 at 15:03
  • \$\begingroup\$ I have down-voted this question because it is not asking for a code review, but it is trying to debate what a best-practice would be. Discussing best-practice is not on-scope for Code Review. There is some code to review, so closing this question is not appropriate though. \$\endgroup\$ – rolfl May 25 '14 at 15:29
3
\$\begingroup\$

Is using a constructor to pass a JPanel, and then adding elements in the new class considered bad coding?

Not necessarily. However, if you only create an instance of the other class just to do things in the constructor, then you don't need the new class - and in that case it is considered bad coding.

If that's all you do in the constructor, then move the code from the constructor to a method (which you can place more or less anywhere, but I'd recommend having it in the original class unless you feel that the original class gets too big).

private void createEntityTab(JPanel panel) {
   ...
   panel.add(...);
   ...
}

Actually, it might be even better to let this method return the JPanel, so that you can use this code:

createPane.addTab("Create Entity", null, createEntityTab());

And use the method:

private JPanel createEntityTab() {
   JPanel panel = new JPanel();
   panel.setLayout(null);
   ...
   panel.add(...);
   ...
   return panel;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I agree that, if all you are doing in the class is in the constructor, then you don't need/want another class, and creating one just for this purpose wouldn't be a best coding practice. Create a class to represent one or more 'things' (concepts, structures, entities, whatever) in your program, not just to hold methods or constructors. \$\endgroup\$ – rcook May 25 '14 at 15:16
  • \$\begingroup\$ @rcook Thank you, that's my point exactly! If the class is not more than a constructor with code, then it should be a method. \$\endgroup\$ – Simon Forsberg May 25 '14 at 15:35
  • \$\begingroup\$ Thanks for your answer! My intention is that the CreateEntity class will contain all the elements (buttons, textfields...) and corresponding action listeners for that tab. This is just to avoid having all the GUI code in one class, which will easily mount up to over 600 lines of code. No matter how tidy I am, I don't like scrolling! Thanks again! \$\endgroup\$ – Jona May 25 '14 at 19:51
1
\$\begingroup\$

I don't see anything wrong with it. You are creating an object that is going to use that data, at construction time, to set itself up. I don't see anything objectionable if that data happens to be a UI Panel. I would think it would be (very slightly) worse to force a programmer to first use the constructor and then use a setter method to do the same thing; conceptually this links the construction with the panel, and it sounds like that fits the model you are making.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I think that technically your answer is correct, but I don't think that is what the OP is doing here. We have understood the question in two different ways, and I think that for each way, both of us is right. As I understand, the CreateEntity class extends Object and only contains a constructor (no fields, no methods, no nothing!) \$\endgroup\$ – Simon Forsberg May 25 '14 at 15:07

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.