Pokemon type evaluator

Question

I thought I would apply what I learned to make something fun. Its purpose is to type check Pokemon, for anyone familiar with the series. It mostly works but I have two concerns:

1. Sometimes you write code and you get the pervading feeling you're not using some method, some API or the best approach available; this is one of those times for me. It definitely doesn't seem like an "if this is the case." Where could it be improved upon?

2. For comments, the last time I posted one of these I was told I should try to write code that is self-evident and doesn't need commenting. I tried that this time, but I feel I'd rather have possibly redundant comments than difficult to follow code that diminishes review quality. I have a commented version but I thought I'd first try the plain one (excepting one to explain a personal habit). What is the general consensus on the best approach?

import java.awt.BorderLayout;
import java.awt.event.ItemEvent;
import java.awt.event.ItemListener;
import java.awt.GridLayout;
import javax.swing.ImageIcon;
import javax.swing.JComboBox;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.WindowConstants;


public class TypeChecker {
    static Double normMulti1 = 1.0, flyMulti1 = 1.0, fightMulti1 = 1.0, fireMulti1 = 1.0,
        waterMulti1 = 1.0, elecMulti1 = 1.0, grassMulti1 = 1.0, bugMulti1 = 1.0,
        poisMulti1 = 1.0, darkMulti1 = 1.0, psyMulti1 = 1.0, ghostMulti1 = 1.0,
        groundMulti1 = 1.0, rockMulti1 = 1.0, steelMulti1 = 1.0, iceMulti1 = 1.0,
        dragMulti1 = 1.0, faeMulti1 = 1.0;

    static String[] elements = {"Normal", "Flying", "Fighting", "Fire", "Water",
        "Electric", "Grass", "Bug", "Poison", "Dark", "Psychic", "Ghost",
        "Ground", "Rock", "Steel", "Ice", "Dragon", "Fairy"};

    static int len = elements.length;
    static Double[] multiplierList1 = new Double[len];

    static JTextField immune = new JTextField(15), res = new JTextField(15),
        vuln = new JTextField(15);

    public static void main(String[] args) {
        JFrame frame = new JFrame("Type Checker");
        JPanel panel = new JPanel(), result = new JPanel();
        result.setLayout(new GridLayout(3, 2, 0, 2));
        result.add(new JLabel("Immune to:"));
        result.add(immune);
        result.add(new JLabel("Resistant to:"));
        result.add(res);
        result.add(new JLabel("Vulnerable to:"));
        result.add(vuln);

        vuln.setEditable(false);
        res.setEditable(false);
        immune.setEditable(false);

        multiplierSet();

        ImageIcon[] types = new ImageIcon[len];
        for (int i = 0; i < len; i++) {
            types[i] = new ImageIcon("Images/" + elements[i] + ".gif");
        }

        JComboBox<ImageIcon> typeList = new JComboBox<ImageIcon>(types);
        typeList.addItemListener(new ItemListener() {
            @Override
            public void itemStateChanged(ItemEvent e) {
                if (e.getStateChange() == ItemEvent.SELECTED) {
                    multiplierReset();
                    multiplierDisplay();

                    for (int i = 0; i < len; i++) {
                        if (e.getItem() == types[i]) {
                            switch(elements[i]) {
                                case "Normal":
                                    fightMulti1 = 2.0;
                                    ghostMulti1 = 0.0;
                                    break;
                                case "Flying":
                                    elecMulti1 = 2.0; iceMulti1 = 2.0; rockMulti1 = 2.0;
                                    bugMulti1 = 0.5; fightMulti1 = 0.5; grassMulti1 = 0.5;
                                    groundMulti1 = 0.0;
                                    break;
                                case "Fighting":
                                    flyMulti1 = 2.0; psyMulti1 = 2.0;
                                    bugMulti1 = 0.5; rockMulti1 = 0.5;
                                    break;
                                case "Fire":
                                    groundMulti1 = 2.0; rockMulti1 = 2.0; waterMulti1 = 2.0;
                                    bugMulti1 = 0.5; faeMulti1 = 0.5; fireMulti1 = 0.5;
                                    grassMulti1 = 0.5; iceMulti1 = 0.5; steelMulti1 = 0.5;
                                    break;
                                case "Water":
                                    elecMulti1 = 2.0; grassMulti1 = 2.0;
                                    fireMulti1 = 0.5; iceMulti1 = 0.5;
                                    steelMulti1 = 0.5; waterMulti1 = 0.5;
                                    break;
                                case "Electric":
                                    groundMulti1 = 2.0;
                                    elecMulti1 = 0.5; flyMulti1 = 0.5; steelMulti1 = 0.5;
                                    break;
                                case "Grass":
                                    bugMulti1 = 2.0; iceMulti1 = 2.0;
                                    fireMulti1 = 2.0; flyMulti1 = 2.0; poisMulti1 = 2.0;
                                    elecMulti1 = 0.5; grassMulti1 = 0.5;
                                    groundMulti1 = 0.5; waterMulti1 = 0.5;
                                    break;
                                case "Bug":
                                    fireMulti1 = 2.0; flyMulti1 = 2.0; rockMulti1 = 2.0;
                                    fightMulti1 = 0.5; grassMulti1 = 0.5; groundMulti1 = 0.5;
                                    break;
                                case "Poison":
                                    groundMulti1 = 2.0; psyMulti1 = 2.0;
                                    bugMulti1 = 0.5; faeMulti1 = 0.5;
                                    fightMulti1 = 0.5; grassMulti1 = 0.5; poisMulti1 = 0.5;
                                    break;
                                case "Dark":
                                    bugMulti1 = 2.0; faeMulti1 = 2.0; fightMulti1 = 2.0;
                                    darkMulti1 = 0.5; ghostMulti1 = 0.5;
                                    psyMulti1 = 0.0;
                                    break;
                                case "Psychic":
                                    bugMulti1 = 2.0; darkMulti1 = 2.0; ghostMulti1 = 2.0;
                                    fightMulti1 = 0.5; psyMulti1 = 0.5;
                                    break;
                                case "Ghost":
                                    darkMulti1 = 2.0; ghostMulti1 = 2.0;
                                    bugMulti1 = 0.5; poisMulti1 = 0.5;
                                    fightMulti1 = 0.0; normMulti1 = 0.0;
                                    break;
                                case "Ground":
                                    iceMulti1 = 2.0; grassMulti1 = 2.0; waterMulti1 = 2.0;
                                    poisMulti1 = 0.5; rockMulti1 = 0.5;
                                    elecMulti1 = 0.0;
                                    break;
                                case "Rock":
                                    fightMulti1 = 2.0; grassMulti1 = 2.0;
                                    groundMulti1 = 2.0; steelMulti1 = 2.0; waterMulti1 = 2.0;
                                    fireMulti1 = 0.5; flyMulti1 = 0.5;
                                    normMulti1 = 0.5; poisMulti1 = 0.5;
                                    break;
                                case "Steel":
                                    fightMulti1 = 2.0; fireMulti1 = 2.0; groundMulti1 = 2.0;
                                    bugMulti1 = 0.5; dragMulti1 = 0.5; faeMulti1 = 0.5;
                                    flyMulti1 = 0.5; grassMulti1 = 0.5; iceMulti1 = 0.5;
                                    normMulti1 = 0.5; psyMulti1 = 0.5;
                                    rockMulti1 = 0.5; steelMulti1 = 0.5;
                                    poisMulti1 = 0.0;
                                    break;
                                case "Ice":
                                    fightMulti1 = 2.0; fireMulti1 = 2.0;
                                    rockMulti1 = 2.0; steelMulti1 = 2.0;
                                    iceMulti1 = 0.5;
                                    break;
                                case "Dragon":
                                    dragMulti1 = 2.0; faeMulti1 = 2.0; iceMulti1 = 2.0;
                                    elecMulti1 = 0.5; fireMulti1 = 0.5;
                                    grassMulti1 = 0.5; waterMulti1 = 0.5;
                                    break;
                                case "Fairy":
                                    poisMulti1 = 2.0; steelMulti1 = 2.0;
                                    bugMulti1 = 0.5; darkMulti1 = 0.5; fightMulti1 = 0.5;
                                    dragMulti1 = 0.0;
                                break; // final break purposely indented such
                            }
                            multiplerSet();
                            multiplierDisplay();
                        }
                    }
                }
            }
        });

        panel.add(typeList);

        frame.setIconImage(new ImageIcon("Images/Icon.png").getImage());
        frame.add(result, BorderLayout.CENTER);
        frame.add(panel, BorderLayout.SOUTH);
        frame.pack();

        frame.setLocationRelativeTo(null);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    public static void multiplierSet() {
        multiplierList1[0] = normMulti1;
        multiplierList1[1] = flyMulti1;
        multiplierList1[2] = fightMulti1;
        multiplierList1[3] = fireMulti1;
        multiplierList1[4] = waterMulti1;
        multiplierList1[5] = elecMulti1;
        multiplierList1[6] = grassMulti1;
        multiplierList1[7] = bugMulti1;
        multiplierList1[8] = poisMulti1;
        multiplierList1[9] = darkMulti1;
        multiplierList1[10] = psyMulti1;
        multiplierList1[11] = ghostMulti1;
        multiplierList1[12] = groundMulti1;
        multiplierList1[13] = rockMulti1;
        multiplierList1[14] = steelMulti1;
        multiplierList1[15] = iceMulti1;
        multiplierList1[16] = dragMulti1;
        multiplierList1[17] = faeMulti1;
    }

    public static void multiplierReset() {
        for (int i = 0; i < len; i++) {
            multiplierList1[i] = 1.0;
        }
    }

    public static void multiplierDisplay() {
        StringBuilder vln = new StringBuilder(), resist = new StringBuilder(),
            imm = new StringBuilder();;

        for (int i = 0; i < len; i++) {
            if (multiplierList1[i] == 0.0) {
                if (imm.toString().isEmpty()) { imm.append(elements[i]); }
                else { imm.append(", " + elements[i]); }
            }
            else if (multiplierList1[i] == 0.5) {
                if (resist.toString().isEmpty()) { resist.append(elements[i]); }
                else { resist.append(", " + elements[i]); }
            }
            else if (multiplierList1[i] == 2) {
                if (vln.toString().isEmpty()) { vln.append(elements[i]); }
                else { vln.append(", " + elements[i]); }
            }
        }

        if (vln.toString().isEmpty()) { vln.append("None"); }
        if (imm.toString().isEmpty()) { imm.append("None"); }
        if (resist.toString().isEmpty()) { resist.append("None"); }
        immune.setText(imm.toString());
        res.setText(resist.toString());
        vuln.setText(vln.toString());
    }
}

Answer 1

Will provide a fuller review if I can later, but anyways here are some quick starters...

See major edit for point 3.

1. Encapsulate the generation of typeList JComboBox as a method so that contents can be easily swapped

   I definitely don't have access to your "Images/" + elements[i] + ".gif", and I don't expect them to be posted here. :D Therefore, in my humble opinion, I think you can put this generation in its own method so that you can easily change the contents without affecting the overall code. For example, in my own testing of your code, I have the following line:

   JComboBox<?> typeList = getComboBox();
   

   And then my implementation goes like the following, to use plain Strings:

   private static JComboBox<?> getComboBox() {
       return new JComboBox<String>( elements );
   }
   

2. switch on the selected item for clarity

   In your current code, you do a for-loop and compare that the selected item is the same object by reference with a very large if-scope (==, which is usually not recommended unless you are really sure of your object referencing) before switching on your 'original' elements array. This sort-of makes sense still in your simpler code example, but it can get messy easily when you scale up for bigger projects as you will have a lot more references to use and keep track. It is therefore easier and clearer to perform the switch on the selected item directly, such that you can even remove the encompassing for and if conditions. For example, I can reduce this:

   for (int i = 0; i < len; i++) {
       if (e.getItem() == types[i]) {
           switch(elements[i]) {
   

   To just switch( e.getItem().toString() ). This is related to the previous point too, since I do not need to rely on your types ImageIcon array.

3. Use enums to represent the various types

   A great deal of your 'calculations' and repeated multiplerSet()/multiplerReset()/multiplerDisplay() method calls can be easily avoided with the right implementation of an enum, e.g. Pokemon.

   public enum Pokemon {
   
       BUG, DARK, DRAGON...;
   
       public static final Map<Pokemon, String> POKEMON_MAP = new EnumMap<>(Pokemon.class);
       private final Set<Pokemon> immuneTo = new HashSet<>();
       private final Set<Pokemon> resistantTo = new HashSet<>();
       private final Set<Pokemon> vulnerableTo = new HashSet<>();
   }
   

   Since the Java naming convention for enum values is ALLCAPS and we want to display a more readable value on the GUI, we can use an EnumMap to store these representations. The three Sets is used to store the appropriate Pokemon types, and we will need to create getter and setter methods for them (using immuneTo as an example):

   public enum Pokemon {
   
       // ... refer to above code
   
       public final Set<Pokemon> getImmuneTo() {
           return Collections.unmodifiableSet(immuneTo);
       }
   
       private Pokemon setImmuneTo(Pokemon... immuneTo) {
           for (final Pokemon pokemon : immuneTo) {
               add(pokemon, this.immuneTo);
           }
           return this;
       }
   
       private void add(final Pokemon type, final Set<Pokemon> set) {
           if (!set.add(type)) {
               throw new IllegalArgumentException("Duplicate addition.");
           }
       }
   }
   

   getImmuneTo is a public method that returns only an unmodifiable view of the underlying Set to prevent accidental changes, which is a good practice especially in this case. Conversely, setImmuneTo is a private method which will be used in the static block below to add Pokemon values at initialization. It conveniently returns itself for method chaining. Finally, the add() method is just used to defend against accidental repetition of values. :) The static block to initialize both the POKEMON_MAP and the values can be done in the following manner:

   public enum Pokemon {
   
       // ... refer to above code
   
       private static void initNameMap() {
           for (final Pokemon pokemon : Pokemon.values()) {
               POKEMON_MAP.put(pokemon, getFormattedName(pokemon));
           }
       }
   
       private static String getFormattedName(final Pokemon pokemon) {
           final String stringName = pokemon.toString();
           final StringBuilder result = new StringBuilder(stringName.substring(0, 1));
           return result.append(stringName.substring(1).toLowerCase()).toString();
       }
   
       static {
           initNameMap();
           BUG.setResistantTo(FIGHTING, GRASS, GROUND).setVulnerableTo(FIRE, FLYING, ROCK);
           DARK.setImmuneTo(PSYCHIC).setResistantTo(DARK, GHOST).setVulnerableTo(BUG, FAIRY, FIGHTING);
           DRAGON.setResistantTo(ELECTRIC, FIRE, GRASS, WATER).setVulnerableTo(DRAGON, FAIRY, ICE);
           // ...
       }
   }
   

   getFormattedName() simply 'converts' the default toString() value for, e.g. "BUG", to "Bug". As mentioned above, method chaining gives us a more fluent approach to specify the types of Pokemon that one is immune, resistant or vulnerable to.

   getComboBox() in your main class can now be simplified as such:

   private static JComboBox<?> getComboBox() {
       return new JComboBox<>( Pokemon.POKEMON_MAP.values().toArray(new String[0]));
   }
   

   Inside your itemStateChanged() method, the code then becomes more readable (some small changes to your JTextField variable names):

   public void itemStateChanged(ItemEvent e) {
       if (e.getStateChange() != ItemEvent.SELECTED) {
           return;
       }
       for (final Entry<Pokemon, String> entry : Pokemon.POKEMON_MAP.entrySet()) {
           if (entry.getValue().equals(e.getItem())) {
               final Pokemon selected = entry.getKey();
               immuneTo.setText(deriveResults(selected.getImmuneTo()));
               resistantTo.setText(deriveResults(selected.getResistantTo()));
               vulnerableTo.setText(deriveResults(selected.getVulnerableTo()));
               break;
           }
       }
   }
   

   deriveResults() is just a method to convert your Set of Pokemon types to a String, and one possible approach is illustrated below:

   private static String deriveResults(final Set<Pokemon> pokemonSet) {
       if (pokemonSet.isEmpty()) {
           return "None";
       }
       StringBuilder results = new StringBuilder();
       for (final Pokemon pokemon : pokemonSet) {
           if (results.length() > 0) {
               results.append(", ");
           }
           results.append(Pokemon.POKEMON_MAP.get(pokemon));
       }
       return results.toString();
   }
   

4. Better use of StringBuilder

   Simply put,

   vln.append( ", " + elements[i] )
   

   should be re-written as

   vln.append( ", " ).append( elements[i] )
   

   If you are appending a single character, do something like builder.append( 'a' ) instead of builder.append( "a" ). If you want to use StringBuilder, use it efficiently.

5. Better use of switch

   break; // final break purposely indented such
   

   This... isn't quite preferred. Normal convention dictates something like this, again for clarity:

   case <n>:
       ...
       break;
   default:
       break;
   

6. Use a better class name

   TypeChecker can imply lots of types checking. You can easily rename it as PokemonTypeChecker. :)

7. Consider a CustomRenderer for your JComboBox?

   I am a little unsure of this suggestion (I have to do some reading for myself as well), but maybe you can consider a CustomRenderer for your JComboBox?

Answer 2

OOP

Java is an object oriented language, but you don't use it like that at all. Most of your code is inside the main method, and you don't have any objects.

Because of this, your program logic is coupled with the presentation of the data, which makes the code hard to read and maintain. Also, if you want to change the logic (eg add a new Type), you have to change the code in a lot of places.

One idea to restructure your code:

• create a constructor for TypeChecker: the initialization of non-final fields should be moved here (eg multiplierList1 = new Double[len];), and most of the code from main as well
• extract multiplier to an enum
• create an AttackType class (to get rid of the multiplierList1 and the various xMulti1 fields which are really hard to maintain)
• don't construct your multipliers in itemStateChanged. Here, you should only get the item that was clicked, and then the display should be changed accordingly

It might look something like this (you can get rid of strength if it is not needed elsewhere):

public class AttackType {

    public static enum MultiplierType {
        NORMAL, FIGHTING, FLYING
        // [...]
    }

    private List<AttackType> vulnerability;
    private List<AttackType> immunity;
    private List<AttackType> resistance;
    private MultiplierType type;
    private Double strength;

    public AttackType() {
        vulnerability = new ArrayList<>();
        immunity = new ArrayList<>();
        resistance = new ArrayList<>();
    }

    public AttackType(MultiplierType type, Double strength) {
        super();
        this.type = type;
        this.strength = strength;
    }

    public void addVulnerability(MultiplierType type, Double strength) {
        vulnerability.add(new AttackType(type, strength));
    }

    public void addResistance(MultiplierType type, Double strength) {
        resistance.add(new AttackType(type, strength));
    }

    public void addImmunity(MultiplierType type, Double strength) {
        immunity.add(new AttackType(type, strength));
    }    

    public String vulnerabilitiesAsString() {
        // return vulns as string
    }
    // [same for immunity and resistance]
}

And then you can construct it like this:

    AttackType fighting = new AttackType(AttackType.MultiplierType.FIGHTING, 0.5);
    fighting.addImmunity(AttackType.MultiplierType.NORMAL, 1.0);
    fighting.addVulnerability(AttackType.MultiplierType.FLYING, 2.0);

either in a utility class, or in main. Then, add all attack types to a list, use that list for your combo box, and display the content of the correct item on change.

This is just one approach, feel free to read up on oop and maybe mvc and see if you can come up with a better one.

Nit-Picking

• Your code doesn't compile, as this line throws an error: if (e.getItem() == types[i]) { (make types final to fix this)
• You don't need to save len = elements.length; in a field (it's not better for performance and too many fields make the code harder to read)
• imm = new StringBuilder();;: remove one semicolon
• vln, imm, vuln, res: these are not great names. Spell them out, and let the name represent what the difference between vln and vuln is.
• multiplierList1: it's not a list, it's an array
• multiplierList1, fightMulti1, etc: why is there a 1 at the end?
• I don't like declaring two variables on one line if the declaration is very long (eg JPanel panel = new JPanel(), result = new JPanel();). It's easy to overlook the second variable
• you mighy think about extracting the data (elements and multiplier) in a xml-configuration file

Comments

Since you asked: Comments are a controversial topic, but here are my thoughts:

Methods and Classes

Methods and Classes should be commented and the comments should express what the method or class does. Everything that is unexpected or any side-effects of a method should be noted here. If there are arguments or return values, they should be described (not just for example // multiplier: the multiplier, but something like multiplier: applied to X, can be Y (for effect A) or Z (for effect B)).

Except for very simple getter and setter and other one-line methods, I would comment all methods.

Code

Inside the methods, it should be commented on how or why the code does what it does, and this should only happen if the code is not clear. Don't comment on simple language features (eg // iterating over elements before a loop), but do comment on unconventional use of language features. If you implement a custom algorithm, describe in short words how it works, if you use an existing algorithm at least write its name in the comments.

If you feel the need to add comments to structure parts of a method into blocks (eg // build frame and panel, // build icons and add listener, // add to frame and display), this can be a sign that your method is too long and does too many things. Extracting the blocks to their own method might be a good solution.

I didn't find any situation in your code where I would have appreciated a comment inside the methods, and if you restructure it a bit they would be even less needed.

Please feel free to post a new ask with comments (and maybe with restructured code) for review if you are unsure about your commenting practices.

Answer 3

As you may have already understood from others answers, you have a lot of data and variables. For instance, your code start with:

static Double normMulti1 = 1.0, flyMulti1 = 1.0, ...

You can notice two things:

1. All your variables have the same value at instantiation. Instead of setting a value for each, you may want to declare first then instantiate. But it will keep a lot of variables.
2. You use these variables among others (element with element_multiplier and element image ...). This should be a big hint that it can be encapsulated as a class. It will allow you to externalise the data, and the processing + calculation. And it will get you a clean and readable code, only exposing functionnality.

I didn't read all your code, just exposing what i caught on first sight. So I will give you a small example to illustrate:

public class HelloWorld{
    public static void main(String []args){
        String[] names = {"bob", "alice", "dave"};
        boolean bobCanRead = true, aliceCanRead = false, daveCanRead = true;
        int ageBob = 42, ageAlice = 2, ageDave = 24;

        for(String name : names) {
            int age; boolean canRead;
            switch(name) {
                case "bob":
                    age = ageBob;
                    canRead = bobCanRead;
                    break;
                case "alice":
                    age = ageAlice;
                    canRead = aliceCanRead;
                    break;
                default:
                    age = ageDave;
                    canRead = daveCanRead;
                    break;
            }
            System.out.print(name);
            System.out.print(canRead ? " can read " : " cannot read ");
            System.out.println("and is " + age + " years old.");
        }
    }
}

As you can see, all these people have common variables. It's a good idea to regroup it, and with a class, the switch won't be necessary as the variables will have a relation (they are all part of 'person').

public class HelloWorld {
    public static void main(String []args){
        // data initialisation
        Person[] persons = {
            new Person("bop", 42, true),
            new Person("alice", 2, false),
            new Person("dave", 24, true),
        };

        // display
        for(Person person : persons) {
            System.out.print(person.name);
            System.out.print(person.canRead ? " can read " : " cannot read ");
            System.out.println("and is " + person.age + " years old.");
        }
    }

    public static class Person {
        public String name;
        public boolean canRead;
        public int age;

        public Person(String name, int age, boolean canRead) {
            this.name = name;
            this.age = age;
            this.canRead = canRead;
        }
    }
}