HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Pokemon type evaluator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
typepokemonevaluator

Problem

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:

-
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?

-
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

Solution

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

See major edit for point 3.

-
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( elements );
}


-
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.

-
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_MAP = new EnumMap<>(Pokemon.class);
    private final Set immuneTo = new HashSet<>();
    private final Set resistantTo = new HashSet<>();
    private final Set 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 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 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() {
retur

Code Snippets

JComboBox<?> typeList = getComboBox();
private static JComboBox<?> getComboBox() {
    return new JComboBox<String>( elements );
}
for (int i = 0; i < len; i++) {
    if (e.getItem() == types[i]) {
        switch(elements[i]) {
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<>();
}
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.");
        }
    }
}

Context

StackExchange Code Review Q#60965, answer score: 16

Revisions (0)

No revisions yet.