Java - Why does it not show an error dialog when the user gives in an unwanted input?

Question

I am trying to color some ovals. The user types in for example color red (rood), and then the circles will be drawn and colored red, that works, but now I am trying to make it give an pop-up if the colors don't match the ones that are the user supposed to give or if the user gives in an number for example, there are three colors which the user can fill in those are rood (red), groen (green), and oranje (orange).

Someone told me I should do it with a try and catch method, which I tried below, when the user gives in for example rood (red) the circles do still work and will be drawn red, but when he gives in for example blue, nothing happens; I want it then to give an error message saying the user needs to give in the three listed colors, and not something else.

this.kleur = this.jTextFieldKleur.getText();
repaint();

if (kleur != null) {
            try {
                if (kleur.equals("rood")) {
                    g.setColor(Color.RED);
                    g.fillOval(75, 60, 35, 35); //links boven
                    g.fillOval(130, 60, 35, 35); //rechts boven
                    g.fillOval(75, 130, 35, 35); //links onder
                    g.fillOval(130, 130, 35, 35); //links onder
                } else if (kleur.equals("groen")) {
                    g.setColor(Color.GREEN);
                    g.fillOval(75, 60, 35, 35); //links boven
                    g.fillOval(130, 60, 35, 35); //rechts boven
                    g.fillOval(75, 130, 35, 35); //links onder
                    g.fillOval(130, 130, 35, 35); //links onder
                } else if (kleur.equals("oranje")) {
                    g.setColor(Color.ORANGE);
                    g.fillOval(75, 60, 35, 35); //links boven
                    g.fillOval(130, 60, 35, 35); //rechts boven
                    g.fillOval(75, 130, 35, 35); //links onder
                    g.fillOval(130, 130, 35, 35); //links onder
                }
            } catch (Exception e) {
                {
                    JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
                }
            }
        }

Answer 1

No exception is thrown here, so the catch clause is pointless. Instead, you could just have this logic in an else block. Note that checking kleur for a null value shouldn't really be a special case. You can work around this by using yoda-notations, or better yet, a switch case:

switch (kleur) {
    case "rood":
        g.setColor(Color.RED);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
        break;
    case "groen":
        g.setColor(Color.GREEN);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
        break;
    case "oranje":
        g.setColor(Color.ORANGE);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
        break;
    default:
        JOptionPane.showMessageDialog
            (null, "Gelieve te kiezen uit rood, oranje of groen.");
}

Answer 2

You are mixing up try..catch and if..else here. An if..else does not throw an exception when no option matches.

Fixing

Try this instead:

if (kleur != null) {
    if (kleur.equals("rood")) {
        g.setColor(Color.RED);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
    } else if (kleur.equals("groen")) {
        g.setColor(Color.GREEN);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
    } else if (kleur.equals("oranje")) {
        g.setColor(Color.ORANGE);
        g.fillOval(75, 60, 35, 35); //links boven
        g.fillOval(130, 60, 35, 35); //rechts boven
        g.fillOval(75, 130, 35, 35); //links onder
        g.fillOval(130, 130, 35, 35); //links onder
    } else {
        JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
    }
}

Refactoring 1

a slightly refactored version follows which works around checking for null by calling the equals method on your constant strings:

if ("rood".equals(kleur)) {
    g.setColor(Color.RED);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
} else if ("groen".equals(kleur)) {
    g.setColor(Color.GREEN);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
} else if ("oranje".equals(kleur)) {
    g.setColor(Color.ORANGE);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
} else {
    JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
}

Refactoring 2

Considering some books suggest returning from a method as quickly as possible, thus making the code easier to follow:

if ("rood".equals(kleur)) {
    g.setColor(Color.RED);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
    return:
}
if ("groen".equals(kleur)) {
    g.setColor(Color.GREEN);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
    return;
}
if ("oranje".equals(kleur)) {
    g.setColor(Color.ORANGE);
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
    return;
}
JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
return;

The last code snippet implies that your method does nothing more than filling the ovals with the color. This limitation is in fact a good thing, since you should keep your methods as small as possible.

Refactoring 3

You can clearly see that your code contains a lot of repetition. We can work around this and also improve extendability. Here comes some experimental, untested code:

private static final Map<String, Color> colorsNL = new HashMap<>();
static {
    colorsNL.put("rood", Color.RED);
    colorsNL.put("groen", Color.GREEN);
    colorsNL.put("oranje", Color.ORANGE);
}

private Shape g = ...

private void setColor(String colorNL) {
    if (!this.colorsNL.contains(colorNL)) {
        JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
        return;
    }
    this.g.setColor(this.colorsNL.get(colorNL));
    this.fillOvals();
}

private void fillOvals() {
    g.fillOval(75, 60, 35, 35); //links boven
    g.fillOval(130, 60, 35, 35); //rechts boven
    g.fillOval(75, 130, 35, 35); //links onder
    g.fillOval(130, 130, 35, 35); //links onder
}

Answer 3

Not sure if I understood you correctly, but don't you want to do something like this?

this.kleur = this.jTextFieldKleur.getText();
repaint();

if (kleur != null) {
            if (kleur.equals("rood")) {
                g.setColor(Color.RED);
                g.fillOval(75, 60, 35, 35); //links boven
                g.fillOval(130, 60, 35, 35); //rechts boven
                g.fillOval(75, 130, 35, 35); //links onder
                g.fillOval(130, 130, 35, 35); //links onder
            } else if (kleur.equals("groen")) {
                g.setColor(Color.GREEN);
                g.fillOval(75, 60, 35, 35); //links boven
                g.fillOval(130, 60, 35, 35); //rechts boven
                g.fillOval(75, 130, 35, 35); //links onder
                g.fillOval(130, 130, 35, 35); //links onder
            } else if (kleur.equals("oranje")) {
                g.setColor(Color.ORANGE);
                g.fillOval(75, 60, 35, 35); //links boven
                g.fillOval(130, 60, 35, 35); //rechts boven
                g.fillOval(75, 130, 35, 35); //links onder
                g.fillOval(130, 130, 35, 35); //links onder
            } else {
                JOptionPane.showMessageDialog(null, "Gelieve te kiezen uit rood, oranje of groen.");
            }
        }
    }

Answer 4

The person who told you that ... gave you wrong input. You only need try/catch if your code is potentially throwing an exception.

In your case, you need to put that error message in a "path" that is used when all of your ifs fail; for example by using a switch statement; where the last case would be default; and that gives you an error. Very much like the nice answer by Mureink is showing you.

But the real takeaway here is: don't trust what other newbies tell you. Instead: when using a concept such as try/catch learn what that thing is about. Never use something just because somebody told you to do so - always understand what you are doing.

In that sense: look here for switch and there for exceptions/try/catch.