11
\$\begingroup\$

I started anew and completely refactored the way I implemented Pokemon type evaluator.

Also, first use of a lambda expression (Case of using it, but still don't fully grasp how it's working). I'm wondering if this is a bit more complex than it needs to be, but otherwise I think these are some significant improvements.

Element enum:

public enum Element {
    NONE("None"),
    NORMAL("Normal"),
    FIRE("Fire"),
    WATER("Water"),
    ELECTRIC("Electric"),
    GRASS("Grass"),
    BUG("Bug"),
    POISON("Poison"),
    ICE("Ice"),
    DRAGON("Dragon"),
    FAIRY("Fairy"),
    STEEL("Steel"),
    ROCK("Rock"),
    GROUND("Ground"),
    FIGHTING("Fighting"),
    FLYING("Flying"),
    PSYCHIC("Psychic"),
    DARK("Dark"),
    GHOST("Ghost");

    public final String name;

    Element(String name) {
        this.name = name;
    }
}

PokemonType class:

    import java.util.HashMap;

public class PokemonType {
    String name;
    HashMap<Element, Double> multipliers = new HashMap<Element, Double>();
    Element element;

    PokemonType(Builder p) {
        name = p.name; multipliers = p.multipliers; element = p.element;
    }

    static class Builder {
        final String name;
        HashMap<Element, Double> multipliers = new HashMap<Element, Double>();
        Element element;

        public Builder(Element element) {
            this.name = element.name;
            this.element = element;
            // Default values
            for (Element e : Element.values()) {
                multipliers.put(e, 1.0);
            }
        }

        public Builder vulnerableTo(Element... elements) {
            for (Element e : elements) { multipliers.put(e, 2.0); }
            return this;
        }
        public Builder immuneTo(Element... elements) {
            for (Element e : elements) { multipliers.put(e, 0.0); }
            return this;
        }
        public Builder resistantTo(Element... elements) {
            for (Element e : elements) { multipliers.put(e, 0.5); }
            return this;
        }
        public PokemonType generate() { return new PokemonType(this); }
    }
    public HashMap<Element, Double> getMultipliers() {
        return this.multipliers;
    }
}

PokemonTypeChecker class, which includes main:

import javax.swing.JCheckBox;
import javax.swing.JComboBox;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.WindowConstants;
import java.awt.BorderLayout;
import java.awt.GridLayout;
import java.awt.event.ItemEvent;
import java.awt.event.ItemListener;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Map;

public class PokemonTypeChecker {

    static PokemonType normal = new PokemonType.Builder(Element.NORMAL)
            .vulnerableTo(Element.FIGHTING).immuneTo(Element.GHOST).generate();

    static PokemonType fire = new PokemonType.Builder(Element.FIRE)
            .vulnerableTo(Element.WATER, Element.GROUND, Element.ROCK)
            .resistantTo(Element.FIRE, Element.GRASS, Element.ICE,
                    Element.BUG, Element.STEEL, Element.FAIRY)
            .generate();

    static PokemonType water = new PokemonType.Builder(Element.WATER)
            .vulnerableTo(Element.ELECTRIC, Element.GRASS)
            .resistantTo(Element.FIRE, Element.WATER, Element.ICE, Element.STEEL)
            .generate();

    static PokemonType electric = new PokemonType.Builder(Element.ELECTRIC)
            .resistantTo(Element.ELECTRIC, Element.STEEL, Element.FLYING)
            .vulnerableTo(Element.GROUND).generate();

    static PokemonType grass = new PokemonType.Builder(Element.GRASS)
            .resistantTo(Element.GRASS, Element.WATER, Element.GROUND, Element.ELECTRIC)
            .vulnerableTo(Element.FIRE, Element.ICE,
                    Element.POISON, Element.FLYING, Element.BUG)
            .generate();

    static PokemonType ice = new PokemonType.Builder(Element.ICE)
            .vulnerableTo(Element.FIRE, Element.STEEL, Element.ROCK, Element.FIGHTING)
            .resistantTo(Element.ICE).generate();

    static PokemonType fighting = new PokemonType.Builder(Element.FIGHTING)
            .vulnerableTo(Element.PSYCHIC, Element.FLYING, Element.FAIRY)
            .resistantTo(Element.BUG, Element.ROCK, Element.DARK)
            .generate();

    static PokemonType poison = new PokemonType.Builder(Element.POISON)
            .resistantTo(Element.GRASS, Element.FIGHTING, Element.POISON,
                    Element.BUG, Element.FAIRY)
            .vulnerableTo(Element.GROUND, Element.PSYCHIC).generate();

    static PokemonType ground = new PokemonType.Builder(Element.GROUND)
            .resistantTo(Element.POISON, Element.ROCK)
            .vulnerableTo(Element.WATER, Element.GRASS, Element.ICE)
            .immuneTo(Element.ELECTRIC).generate();

    static PokemonType flying = new PokemonType.Builder(Element.FLYING)
            .resistantTo(Element.GRASS, Element.FIGHTING, Element.BUG)
            .vulnerableTo(Element.ELECTRIC, Element.ICE, Element.ROCK)
            .immuneTo(Element.GROUND).generate();

    static PokemonType psychic = new PokemonType.Builder(Element.PSYCHIC)
            .vulnerableTo(Element.BUG, Element.DARK, Element.GHOST)
            .resistantTo(Element.FIGHTING, Element.PSYCHIC).generate();

    static PokemonType bug = new PokemonType.Builder(Element.BUG)
            .vulnerableTo(Element.FIRE, Element.FLYING, Element.ROCK)
            .resistantTo(Element.GRASS, Element.FIGHTING, Element.GROUND)
            .generate();

    static PokemonType rock = new PokemonType.Builder(Element.ROCK)
            .vulnerableTo(Element.WATER, Element.GRASS, Element.FIGHTING,
                    Element.GROUND, Element.STEEL)
            .resistantTo(Element.NORMAL, Element.FIRE, Element.POISON, Element.FLYING)
            .generate();

    static PokemonType ghost = new PokemonType.Builder(Element.GHOST)
            .vulnerableTo(Element.GHOST, Element.DARK)
            .resistantTo(Element.BUG, Element.POISON)
            .immuneTo(Element.NORMAL, Element.FIGHTING)
            .generate();

    static PokemonType dragon = new PokemonType.Builder(Element.DRAGON)
            .vulnerableTo(Element.ICE, Element.DRAGON, Element.FAIRY)
            .resistantTo(Element.WATER, Element.FIRE, Element.ELECTRIC, Element.GRASS)
            .generate();

    static PokemonType dark = new PokemonType.Builder(Element.DARK)
            .vulnerableTo(Element.FIGHTING, Element.BUG, Element.FAIRY)
            .resistantTo(Element.GHOST, Element.DARK)
            .immuneTo(Element.PSYCHIC).generate();

    static PokemonType steel = new PokemonType.Builder(Element.STEEL)
            .vulnerableTo(Element.FIRE, Element.FIGHTING, Element.GROUND)
            .resistantTo(Element.NORMAL, Element.GRASS, Element.ICE, Element.FLYING,
                    Element.PSYCHIC, Element.BUG, Element.ROCK, Element.DRAGON,
                    Element.STEEL, Element.FAIRY)
            .immuneTo(Element.POISON).generate();

    static PokemonType fairy = new PokemonType.Builder(Element.FAIRY)
            .vulnerableTo(Element.STEEL, Element.POISON)
            .resistantTo(Element.FIGHTING, Element.DARK, Element.BUG)
            .immuneTo(Element.DRAGON).generate();

    static PokemonType none = new PokemonType.Builder(Element.NONE).generate();

    static ArrayList<PokemonType> pokemonTypes = new ArrayList<>(Arrays.asList(normal,
            fighting, flying, fire, water, electric, grass, bug, poison, steel, ground,
            rock, ice, dragon, fairy, psychic, dark, ghost, none));

    static JTextField immunities = new JTextField(15),
            resistances = new JTextField(15), vulnerabilities = new JTextField(15);

    public static void main(String[] args) {
        JFrame frame = new JFrame("Pokemon Type Checker");
        JPanel selectionPanel = new JPanel(), result = new JPanel();
        result.setLayout(new GridLayout(3, 2, 0, 2));

        immunities.setEditable(false);
        resistances.setEditable(false);
        vulnerabilities.setEditable(false);

        result.add(new JLabel("Immune to:"));
        result.add(immunities);
        result.add(new JLabel("Resistant to:"));
        result.add(resistances);
        result.add(new JLabel("Vulnerable to:"));
        result.add(vulnerabilities);

        JComboBox<String> typeList = new JComboBox<String>();
        JComboBox<String> typeList2 = new JComboBox<String>();

        for (Element e : Element.values()) {
            typeList.addItem(e.name);
            typeList2.addItem(e.name);
        }

        typeList.setSelectedItem(Element.NORMAL.name);

        ItemListener typeListener = e -> {
            PokemonType primaryPokemonType = normal, secondaryPokemonType = none;
            if (e.getStateChange() == ItemEvent.SELECTED) {
                displayAttributes(getPokemonType((String)typeList.getSelectedItem()),
                        getPokemonType((String)typeList2.getSelectedItem())
                );
            }
        };

        typeList.addItemListener(typeListener);
        typeList2.addItemListener(typeListener);

        selectionPanel.add(typeList);
        selectionPanel.add(typeList2);

        frame.add(result, BorderLayout.CENTER);
        frame.add(selectionPanel, BorderLayout.SOUTH);

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

    public static void displayAttributes(PokemonType primary, PokemonType secondary) {
        if (primary == secondary) { secondary = none; }

        immunities.setText("");
        vulnerabilities.setText("");
        resistances.setText("");
        StringBuilder immuneBuilder = new StringBuilder(""),
                vulnerableBuilder = new StringBuilder(""),
                resistBuilder = new StringBuilder("");

        for(Map.Entry<Element, Double> entry : primary.getMultipliers().entrySet()){
            for(Map.Entry<Element, Double> entry2 : secondary.getMultipliers().entrySet()) {
                if (entry.getKey() == entry2.getKey()) {

                    if (entry.getValue() * entry2.getValue() == 0.0) {
                        immuneBuilder.append(entry.getKey().name).append(", ");
                    }
                    else if (entry.getValue() * entry2.getValue()  <= 0.5 ) {
                        resistBuilder.append(entry.getKey().name).append(", ");
                    }
                    else if (entry.getValue() * entry2.getValue()  >= 2.0) {
                        vulnerableBuilder.append(entry.getKey().name).append(", ");
                    }
                }
            }
        }

        finalizeBuilder(immuneBuilder);
        finalizeBuilder(resistBuilder);
        finalizeBuilder(vulnerableBuilder);

        immunities.setText(immuneBuilder.toString());
        vulnerabilities.setText(vulnerableBuilder.toString());
        resistances.setText(resistBuilder.toString());
    }

    public static PokemonType getPokemonType(String name) {
        for (PokemonType p : pokemonTypes) {
            if (p.element.name == name) {
                return p;
            }
        }
        return none; // should never be the case
    }
    // To make sure that the field says if nothing applies
    public static StringBuilder finalizeBuilder(StringBuilder sb) {
        if (sb.length() == 0) { sb.append("None."); }
        else {
            // Just to replace the last comma with a period
            sb.setLength(sb.length() - 2);
            sb.append('.');
        }
        return sb;
    }
}
\$\endgroup\$
8
\$\begingroup\$

I think the overall design is good, although make the main method create a new PokemonTypeChecker and then move all the stuff currently in the main method to a run method or similar - doing logic in the constructor is bad practice, and not writing OO code (i.e. making everything static) is too. You could also perhaps consider JavaFX over swing/awt too.

Comments:

  • It seems like Type may be a better name than Element?
  • It's good practice to make everything as private as possible, so consider making the Element constructor private.
  • Override toString in the enumerated type instead of having to specify a name in the constructor (you could utilise name).
  • It's good practice to make classes that shouldn't be extended final.

  • Use the diamond operator to make it quicker to change the left hand side of a generic class, and faster to write.

  • Use the least-specific (but relevant) parent class on the LHS to make it quicker to change and, usually, write (as stated in Effective Java).

Old:

HashMap<Element, Double> multipliers = new HashMap<Element, Double>();

New:

Map<Element, Double> multipliers = new HashMap<>();
  • Is there a reason you're using JTextFields over a JLabel or similar? You're preventing them from being edited anyway.
  • You might as well initialise empty StringBuilders instead of specifying the empty string literal in the constructor (they're identical).
  • If the return none; part of the getPokemonType method should never happen, then you probably want to throw an unchecked exception there (likely IllegalArgumentException).
  • Looks like the you could make use of Stream#filter in the displayAttributes.

Possibly subjective points I wanted to include regardless:

  • Conventionally the method is called "build" rather than generate.
  • I prefer Guava's way of instantiating builders because then you don't need to type the inner class (MyBuilder.Builder) and it's shorter.
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Welcome to Code Review, very nice answer you got here! I'm sure you'll feel right at home! \$\endgroup\$ Sep 11 '14 at 23:10
  • 1
    \$\begingroup\$ Ah on the toString suggestion, the Elements each contain images. I just edited them as strings so a reviewer could compile the code. I actually should make a point to learn JavaFX...or at least understand why I should, thank you. \$\endgroup\$
    – Legato
    Sep 12 '14 at 1:42
  • 1
    \$\begingroup\$ and now all my visual code is JavaFx, haha. Still need to actually apply FXML more though. Thank you again @Toby \$\endgroup\$
    – Legato
    Oct 22 '15 at 6:30
  • \$\begingroup\$ No problem @Legato - glad to see you are continuing to improve your project :-) \$\endgroup\$
    – Toby
    Oct 22 '15 at 22:36

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.