Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Alright, so my code is 'all over' and missing some fundamentals as regarding to the MVC design pattern.I should separate my concerns and I don't have a model layer and i'm clueless to how to implement it in the current situation. My two classes are :

public class GuiPanel extends JPanel {

public ImageIcon resource01;

public ImageIcon getResource01() {
    if (resource01 == null) {
        resource01 = new ImageIcon(getClass().getResource("/images/reptiles99_11.jpg"));
    }
    return resource01;
}

public ImageIcon resource02;

public ImageIcon getResource02() {
    if (resource02 == null) {
        resource02 = new ImageIcon(getClass().getResource("/images/rsz_s13.jpg"));
    }
    return resource02;
}

private Font font;

@Override
public Font getFont() {
    if (font == null) {
        font = new Font("Times new Roman", Font.BOLD, 14);
    }
    return font;
}

private final String[] listS1 = {"-Odaberi-", "Akutni", "Hronicni"};
private int counterS1 = 0;
private JComboBox comboS1;

private JComboBox getComboS1() {
    if (comboS1 == null) {
        comboS1 = new JComboBox();
        comboS1.setBounds(245, 135, 90, 30);
        for (int i = 0; i < 3; i++) {
            comboS1.addItem(listS1[counterS1++]);
        }
        add(comboS1);

    }
    return comboS1;
}

private final String[] listS2 = {"-Odaberi-", "Abdomen", "Udovi", "Glava"};
private int counterS2 = 0;
private JComboBox comboS2;

private JComboBox getComboS2() {
    if (comboS2 == null) {
        comboS2 = new JComboBox();
        comboS2.setBounds(245, 235, 90, 30);
        for (int i = 0; i < 4; i++) {
            comboS2.addItem(listS2[counterS2++]);
        }
        add(comboS2);

    }
    return comboS2;
}

private ComboBoxModel[] models;

private ComboBoxModel[] getComboModels() {
    if (models == null) {
        models = new ComboBoxModel[5];
        models[0] = new DefaultComboBoxModel(new String[]{"-Odaberi-"});
        models[1] = new DefaultComboBoxModel(new String[]{"-Odaberi-", "1 - GL",
            "2 - GS", "3 - GD", "4 - SL", "5 - SD", "6 - SL", "7 - DL", "8 - DS", "9 - DD"});

    }
    return models;
}

private JComboBox comboS3;

private JComboBox getComboS3() {
    if (comboS3 == null) {
        comboS3 = new JComboBox();
        comboS3.setBounds(245, 335, 90, 30);
        comboS3.setModel(models[0]);
        comboS3.disable();
        add(comboS3);

    }
    return comboS3;
}

private JLabel title;

private JLabel getTitleLabel() {
    if (title == null) {
        title = new JLabel("Aplikacija za dijagnozu bolesti");
        title.setBounds(90, 40, 200, 100);
        title.setFont(getFont());
        add(title);
    }

    return title;
}

private JLabel diagnose;

private JLabel getDiagnoseLabel() {
    if (diagnose == null) {
        diagnose = new JLabel("Preliminarna dijagnoza :");
        diagnose.setBounds(10, 420, 200, 100);
        add(diagnose);

    }
    return diagnose;
}

private JLabel symptome01;

private JLabel getSymptome01Label() {
    if (symptome01 == null) {
        symptome01 = new JLabel("Vrsta bola koju osecate : ");
        symptome01.setBounds(10, 100, 200, 100);
        add(symptome01);
    }

    return symptome01;
}

private JLabel symptome02;

private JLabel getSymptome02Label() {
    if (symptome02 == null) {
        symptome02 = new JLabel("U kom delu tela osecate taj bol :");
        symptome02.setBounds(10, 200, 200, 100);
        add(symptome02);
    }
    return symptome02;
}

private JLabel symptome03;

private JLabel getSymptome03Label() {
    if (symptome03 == null) {
        symptome03 = new JLabel("Vas bol osecate u (vidi sliku) : ");
        symptome03.setBounds(10, 300, 200, 100);
        add(symptome03);
    }
    return symptome03;
}

private JLabel picture01;

private JLabel getPicture01Label() {
    if (picture01 == null) {
        picture01 = new JLabel(getResource01());
        picture01.setBounds(400, 40, 350, 400);
        add(picture01);

    }
    return picture01;
}

private JLabel picture02;

private JLabel getPicture02Label() {
    if (picture02 == null) {
        picture02 = new JLabel(getResource02());
        picture02.setBounds(400, 40, 350, 400);
        picture02.setVisible(false);
        add(picture02);
    }
    return picture02;

}

private JButton reset;

private JButton getResetButton() {
    if (reset == null) {
        reset = new JButton("Ponovo");
        reset.setBounds(640, 470, 110, 50);
        reset.addActionListener(new AgainButton(getResetButton(), getComboS1(),
                getComboS2(), getComboS3(), getDiagnoseField(), getPicture01Label()));
        add(reset);
    }

    return reset;
}

private JButton calculate;

private JButton getCalculateButton() {
    if (calculate == null) {
        calculate = new JButton("Dijagnoza");
        calculate.setBounds(400, 470, 110, 50);
        calculate.addActionListener(new CalculateButton(getCalculateButton(),
                getComboS1(), getComboS2(), getComboS3(), getDiagnoseField()));
        add(calculate);
    }
    return calculate;
}

private JTextField diagnoseField;

private JTextField getDiagnoseField() {
    if (diagnoseField == null) {
        diagnoseField = new JTextField("");
        diagnoseField.setBounds(10, 490, 350, 30);
        diagnoseField.setEditable(false);
        add(diagnoseField);
    }
    return diagnoseField;
}

public GuiPanel() {

    setLayout(null);

    getComboS1();
    getComboS2();
    getComboModels();
    getComboS3();
    getTitleLabel();
    getDiagnoseLabel();
    getSymptome01Label();
    getSymptome02Label();
    getSymptome03Label();
    getPicture01Label();
    getPicture02Label();
    getResetButton();
    getCalculateButton();
    getDiagnoseField();
}

This is the main GUI class and i have a button function class which implements an action listener :

public class CalculateButton implements ActionListener {

private final JComboBox comboS1;
private final JComboBox comboS2;
private final JComboBox comboS3;
private final JTextField diagnoseField;

public CalculateButton(JButton calculate, JComboBox comboS1, JComboBox comboS2, JComboBox comboS3, JTextField diagnoseField) {
    this.comboS1 = comboS1;
    this.comboS2 = comboS2;
    this.comboS3 = comboS3;
    this.diagnoseField = diagnoseField;
    calculate.addActionListener(this);
}

@Override
public void actionPerformed(ActionEvent e) {

    if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 1) {
        diagnoseField.setText("Upala zucovoda");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 2) {
        diagnoseField.setText("Cir na zeludcu");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 3) {
        diagnoseField.setText("Gastritis");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 4) {
        diagnoseField.setText("Renalne kolike");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 5) {
        diagnoseField.setText("Divertikulitis");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 6) {
        diagnoseField.setText("Rani apendicitis / pankreatitis");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 7) {
        diagnoseField.setText("Apendicitis");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 8) {
        diagnoseField.setText("Cistitis / bolovi u rektumu / problemi sa stolicom");
    } else if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 9) {
        diagnoseField.setText("Spasma stigme");
    }

}

}

All the strings, although in my native language, should be viewed as plain strings.

share|improve this question
up vote 10 down vote accepted

Layout

Don't hard-code dimensions and positions. On a Mac, everything looks wrong. (I don't have your images, so I'm going to ignore them for the rest of this review.)

Screenshot on Mac

Furthermore, the user of the GuiPanel has no clue how large it needs to be, and will also be stuck hard-coding its dimensions.

A better way to do layout is using LayoutManagers. The following result was attained more or less automatically, after tweaking just the size of the buttons and some margins.

Result using LayoutManagers

GUI construction

You use the lazy getter pattern a lot. Why? You gain nothing from laziness, as you will certainly need to instantiate every component of this UI. Furthermore, it's very weird that your constructor is calling these getters purely for their side-effect. Rather, you should be taking care of all the instantiation in the constructor.

There is no point in making an instance variable for every JLabel. Local variables in the constructor will do: once each label has been added to the layout, you'll never need to refer to it again.

I would construct the UI this way:

import java.awt.*;
import javax.swing.*;

public class GuiPanel extends JPanel {
    private JComboBox<String> typeCombo = makeComboBox("Akutni", "Hronicni"),
                              partCombo = makeComboBox("Abdomen", "Udovi", "Glava"),
                              painCombo = makeComboBox("1 - GL", "2 - GS", "3 - GD", "4 - SL", "5 - SD", "6 - SL", "7 - DL", "8 - DS", "9 - DD");

    private JTextField diagnosisField = new JTextField(20);

    public GuiPanel() {
        this.setLayout(new BorderLayout());
        this.setBorder(BorderFactory.createEmptyBorder(20, 20, 20, 20));

        // Title
        JLabel title = new JLabel("Aplikacija za dijagnozu bolesti", SwingConstants.CENTER);
        title.setFont(new Font("Times new Roman", Font.BOLD, 14));
        this.add(title, BorderLayout.NORTH);

        // Questions
        JPanel questions = new JPanel();
        questions.setLayout(new GridLayout(0, 2, 30, 30));
        questions.setBorder(BorderFactory.createEmptyBorder(20, 20, 20, 20));
        questions.add(new JLabel("Vrsta bola koju osecate :"));
        questions.add(this.typeCombo);
        questions.add(new JLabel("U kom delu tela osecate taj bol :"));
        questions.add(this.partCombo);
        questions.add(new JLabel("Vas bol osecate u (viki sliku) :"));
        //this.painCombo.setEnabled(false);
        questions.add(this.painCombo);
        this.add(questions, BorderLayout.CENTER);

        // Diagnosis and buttons
        JPanel bottom = new JPanel();
        bottom.setLayout(new FlowLayout());
        bottom.setAlignmentY(Container.BOTTOM_ALIGNMENT);

        JPanel diagnosis = new JPanel();
        diagnosis.setLayout(new BoxLayout(diagnosis, BoxLayout.Y_AXIS));
        diagnosis.setAlignmentX(Container.LEFT_ALIGNMENT);
        diagnosis.add(new JLabel("Preliminarna dijagnoza :"));
        diagnosisField.setEditable(false);
        diagnosis.add(diagnosisField);
        bottom.add(diagnosis);

        JButton diagnosisButton = new JButton("Dijagnoza");
        diagnosisButton.setMargin(new Insets(10, 10, 10, 10));
        diagnosisButton.addActionListener(ActionEvent -> { diagnose(); });
        bottom.add(diagnosisButton);

        JButton resetButton = new JButton("Ponovo");
        resetButton.setMargin(new Insets(10, 10, 10, 10));
        resetButton.addActionListener(ActionEvent -> { reset(); });
        bottom.add(resetButton);

        this.add(bottom, BorderLayout.SOUTH);
    }

    private static JComboBox<String> makeComboBox(String... options) {
        JComboBox<String> combo = new JComboBox<String>();
        combo.addItem("-Odaberi-");
        for (String opt : options) {
            combo.addItem(opt);
        }
        return combo;
    }

    private void diagnose() {
        String disease = DiagnosisEngine.diagnose(
            this.typeCombo.getSelectedIndex() == 0 ? null : (String)this.typeCombo.getSelectedItem(),
            this.partCombo.getSelectedIndex() == 0 ? null : (String)this.partCombo.getSelectedItem(),
            this.painCombo.getSelectedIndex()
        );
        this.diagnosisField.setText(disease == null ? "" : disease);
    }

    private void reset() {
        this.typeCombo.setSelectedIndex(0);
        this.partCombo.setSelectedIndex(0);
        this.painCombo.setSelectedIndex(0);
        this.diagnose();
    }

}

Diagnosis

The diagnosis logic should not be coupled with the UI at all. Since this diagnosis task is simple, let the GuiPanel extract the necessary information from the UI, ask for a diagnosis, and display the result.

public class DiagnosisEngine {
    public static String diagnose(String type, String part, int pain) {
        if ("Akutni".equals(type)) {
            if ("Abdomen".equals(part)) {
                switch (pain) {
                    case 1: return "Upala zucovoda";
                    case 2: return "Cir na zeludcu";
                    case 3: return "Gastritis";
                    case 4: return "Renalne kolike";
                    case 5: return "Divertikulitis";
                    case 6: return "Rani apendicitis / pankreatitis";
                    case 7: return "Apendicitis";
                    case 8: return "Cistitis / bolovi u rektumu / problemi sa stolicom";
                    case 9: return "Spasma stigme";
                }
            }
        }
        return null;
    }

}
share|improve this answer
    
Thank you for actually taking the time to rewrite and reorganize the logic of the code. It really helps out since i'm only starting out in coding... My next step was to implement more panels and learn on how to use border layouts. I'll read up on that and try to figure out the logic presented. I'm sure, regarding your reputation and such, that this is the proper way to do things and i'll follow this example in my future coding. – NxS Feb 12 at 9:30
    
With lazy initialization you gain two advantages: Having the initialization of each component at ONE place. Furthermore you are decoupled from "when" the component ist created (if you only use the getters). This helps you when rearranging the UI. You do not depend on the order of initialization of your components. Just use the getters and you will get the instance, does'nt matter if it was just created or already available. – oopexpert Feb 12 at 9:38
    
I did not go into platform independency. You got a good point from the usability perspective. – oopexpert Feb 12 at 9:41

General

This looks much better as the initiallization of every component is glued together and at one place.

Adding components

The components should not add themselves to the parent. The should not know the parent explicitly. Add them in the constructor like this:

public GuiPanel() {

    ...

    add(getComboS1());
    add(getComboS2());
    add(getComboModels());
    add(getComboS3());

    ...

}

Model

In a similar question I provided an example. As a matter of revisiong I will provide it here as well so you get an idea how to separate the concerns.

Main.java

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {

            @Override
            public void run() {
                MyModel myModel = new MyModel();
                MyFrame myFrame = new MyFrame(myModel);
                myFrame.setVisible(true);
            }

        });
    }

MyFrame.java

import java.awt.BorderLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JTextField;

public class MyFrame extends JFrame {

    private final MyModel myModel;

    private JButton nextWordButton;
    private WordTextField wordTextField;


    public MyFrame(MyModel myModel) {
        this.myModel = myModel;
        setDefaultCloseOperation(DISPOSE_ON_CLOSE);
        getContentPane().setLayout(new BorderLayout());
        getContentPane().add(getNextWordButton(), BorderLayout.WEST);
        getContentPane().add(getWordTextField(), BorderLayout.CENTER);
        setSize(400, 200);
    }


    private JButton getNextWordButton() {
        if (nextWordButton == null) {
            nextWordButton = new JButton("Next");
            nextWordButton.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {
                    myModel.execute();
                }
            });
        }
        return nextWordButton;
    }


    private WordTextField getWordTextField() {
        if (wordTextField == null) {
            wordTextField = new WordTextField();
            myModel.add(wordTextField);
        }
        return wordTextField;
    }

}

MyModel.java

import java.util.HashSet;
import java.util.Set;

public class MyModel {


    private static final String[] INFINITE_SENTENCE = new String[] {"This", "sentence", "will", "never", "end", "because ..."};


    private int index;
    private Set<MyModelListener> listeners;

    public MyModel() {
        this.index = 0;
        this.listeners = new HashSet<>();
    }

    public void execute() {
        this.index++;
        notifyOnExecute(getWord());
    }

    private String getWord() {
        return INFINITE_SENTENCE[this.index % INFINITE_SENTENCE.length];
    }

    private void notifyOnExecute(String word) {

        for (MyModelListener listener: this.listeners) {
            listener.onExecute(word);
        }

    }

    public boolean add(MyModelListener l) {
        l.onListenerRegistration(getWord());
        return listeners.add(l);
    }

    public boolean remove(MyModelListener l) {
        return listeners.remove(l);
    } 


}

MyModelListener.java

public interface MyModelListener {

    void onExecute(String word);

    void onListenerRegistration(String word);

}

WordTextField

public class WordTextField extends JTextField implements MyModelListener {

    @Override
    public void onExecute(String word) {
        setTextLater(word);
    }


    private void setTextLater(String word) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                setText(word);
            }
        });
    }


    @Override
    public void onListenerRegistration(String word) {
        setTextLater(word);
    }

}

The procedure is:

  1. The Button is clicked
  2. ActionListener is called
  3. It is delegating to the Model
  4. The Model changes its internals
  5. The Model informs interested Listeners of internal changes (Listener pattern, push mechanism)
  6. As the WordTextField was registered as interested in Model changes it will receive the notification an can update its state on its own

The main advantages are:

  1. Decoupling receiver from sender. The WordTextField does not need to know if the action was initiated by a button. It could have been anything that triggers the model change.
  2. Decoupling UI from Model, so you do not bind yourself to a specific UI technology anymore. Throw away Swing and use JavaFX. This will also work. If you do not see any necessity to change your UI technology it remains decoupled code. The model will be testable without any visuals.
share|improve this answer
    
Your Main is wrong. It violates the fundamental lack of thread safety in Swing. You should call invokeLater. – Boris the Spider Feb 12 at 12:47
    
Further, PropertyChangeSupport is provided so you don't have to handle maintaining lists of listeners and deal with the memory leaks that that inevitably creates. I'm going to have to downvote this answer. – Boris the Spider Feb 12 at 12:49
    
I want to notice that the java.beans.PropertyChangeSupport is meant to be used in the context of beans. The Model is not a bean. Furthermore it is meant to have support for bound properties. And there is no property where I want to provide binding support. Please have a look at the Java docs. – oopexpert Feb 12 at 13:15
    
Also the listener interface has a real semantic of the problem domain that you cannot provide with PropertyChangeSupport. There every property is independent of the others. For Beans it's by definition true. But not for real objects. So you may have a more complex and domain specific communication model that a standard Listener is able to provide. – oopexpert Feb 12 at 13:23
    
On the thread-safty point you are totally right. I adapted my example. On the same argument setting the text the class "WordTextField" has to be adapted as well. – oopexpert Feb 12 at 13:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.