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 = 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());
}
}
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
typeListJComboBoxas a method so that contents can be easily swappedI definitely don’t have access to your
"Images/" + elements[i] + ".gif", and I don’t expect them to be posted here. 😀 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 ); } -
switchon the selected item for clarityIn your current code, you do a
for-loop and compare that the selected item is the same object by reference with a very largeif-scope (==, which is usually not recommended unless you are really sure of your object referencing) before switching on your ‘original’elementsarray. 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 theswitchon the selected item directly, such that you can even remove the encompassingforandifconditions. 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 yourtypesImageIconarray. -
Use
enums to represent the various typesA great deal of your ‘calculations’ and repeated
multiplerSet()/multiplerReset()/multiplerDisplay()method calls can be easily avoided with the right implementation of anenum, 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
enumvalues isALLCAPSand we want to display a more readable value on the GUI, we can use anEnumMapto store these representations. The threeSets is used to store the appropriatePokemontypes, and we will need to create getter and setter methods for them (usingimmuneToas 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."); } } }getImmuneTois a public method that returns only an unmodifiable view of the underlyingSetto prevent accidental changes, which is a good practice especially in this case. Conversely,setImmuneTois a private method which will be used in thestaticblock below to addPokemonvalues at initialization. It convenientlyreturns itself for method chaining. Finally, theadd()method is just used to defend against accidental repetition of values. 🙂 Thestaticblock to initialize both thePOKEMON_MAPand 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 defaulttoString()value for, e.g."BUG", to"Bug". As mentioned above, method chaining gives us a more fluent approach to specify the types ofPokemonthat 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 yourJTextFieldvariable 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 yourSetofPokemontypes to aString, 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(); } -
Better use of
StringBuilderSimply 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 ofbuilder.append( "a" ). If you want to useStringBuilder, use it efficiently. -
Better use of
switchbreak; // final break purposely indented suchThis… isn’t quite preferred. Normal convention dictates something like this, again for clarity:
case <n>: ... break; default: break; -
Use a better class name
TypeCheckercan imply lots of types checking. You can easily rename it asPokemonTypeChecker. 🙂 -
Consider a
CustomRendererfor yourJComboBox?I am a little unsure of this suggestion (I have to do some reading for myself as well), but maybe you can consider a
CustomRendererfor yourJComboBox?
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 (egmultiplierList1 = new Double[len];), and most of the code frommainas well - extract multiplier to an enum
- create an AttackType class (to get rid of the
multiplierList1and the variousxMulti1fields 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]) {(maketypesfinal 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 semicolonvln,imm,vuln,res: these are not great names. Spell them out, and let the name represent what the difference betweenvlnandvulnis.multiplierList1: it’s not a list, it’s an arraymultiplierList1,fightMulti1, etc: why is there a1at 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.
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:
- 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.
- 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;
}
}
}