Button-clicking UI for a game

I've been working on this game for a couple weeks now as a purely personal project to help myself learn Java. I'd like to know if I'm on the right track with my project.

Items I'd like specific feedback on:

• GridBagLayout - I think I'm working this out okay?

• Classes - Do I need to break parts of this up into other pieces?

• Because the numbers could potentially increment past Integer values, I was advised to use BigInteger. The way that I did that, is that good practice?

• Any other suggestions are welcomed.

I look forward to what you have to say.

```
package DamageDealer;
import javax.swing.*;
import java.awt.*;
import java.awt.GridBagLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;
import java.io.*;
import java.math.*;
import java.util.ArrayList;

public class Take1 {

//Constants
//Variables and conversions from string to big integer for math purposes
int intBIResult = 0; //Used for BigInteger comparisons
String stringConstantOne = "1";
String stringConstantTen = "10";
String stringConstantHundred = "100";
String stringConstantThousand = "1000";
String stringConstantTenThousand = "10000";
String stringConstantHundredThousand = "100000";

BigInteger biConstantOne = new BigInteger(stringConstantOne);
BigInteger biConstantTen = new BigInteger(stringConstantTen);
BigInteger biConstantHundred = new BigInteger(stringConstantHundred);
BigInteger biConstantThousand = new BigInteger(stringConstantThousand);
BigInteger biConstantTenThousand = new BigInteger(stringConstantTenThousand);
BigInteger biConstantHundredThousand = new BigInteger(stringConstantHundredThousand);

//Point variables
//Values and conversions from string to big integer
String stringDamageOutput = "0";
String stringEPoints = "0";
String stringNewEPoints = "0";
String stringSpentEPoints = "0";
int intTo

I haven't worked with Java in a long time, so I can't say much about language-specific issues or whether your use of AWT makes sense. But here is what I can say:

Constants

You've got several 'constants' there, several of which (the strings) are only used to initialize other 'constants'. The extra indirection makes your code harder to read. Also, constant names are usually written in UPPERCASE, and they're normally marked as public static final - static, because you don't need a copy for each instance of your class, and final, because you don't want constants to change.

So just write public static final BigInteger THOUSAND = new BigInteger("1000");. Note that you can also make use of a few existing BigInteger constants: BigInteger.ZERO, BigInteger.ONE and BigInteger.TEN.

There's another problem with those constants however: they are actually used as the prices of units, so either their names are misleading or their use is incorrect.

The load and save data code repeats the savegame filename several times - this is where a constant may actually be useful (assuming you don't plan on supporting multiple savegame slots).

Take1 class

Take1 is an undescriptive name. Game may be a better name, but it depends on what its intended purpose is.

A constructor that's starting a thread, with important game-related functionality stuffed in an anonymous class is... peculiar. Constructors should generally not have side-effects - their purpose is to initialize an object. I would move that code to a StartGame method, which can then be called from main.

run is doing too much: it's asking whether the user wants to start a new game or load an existing game, it's also starting the game, and finally it's saving the game. Those are 3 separate things that each deserve their own method. This helps keeping your code manageable.

Your dialog result handling code contains the same load-game code 4 times. It can be rewritten to only contain that code once. For example, the else if (result == JOptionPane.NO_OPTION) and else clauses are doing the same thing, so the else if clause can safely be removed.

You do have a loadGameData method. Why is there no matching saveGameData method? And why are these important methods located in an anonymous class? This sort of thing makes code hard to reuse and test.

addComponentsToPane class

The game logic is heavily intertwined with UI-related code. It's hard to determine how the game works from just looking at this code. Again, the class name is undescriptive - it actually contains both game-logic and UI code, but the name only suggests the latter.

The btnDamage click handler contains a lot of duplicated code. The nested if-else statements can be replaced with a few if statements:

if (unit1Count.compareTo(BigInteger.ZERO) > 0) {
    damage = damage.add(unit1Count.multiply(unit1Damage));
}
if (unit2Count.compareTo(BigInteger.ZERO) > 0) {
    damage = damage.add(unit2Count.multiply(unit2Damage));
}
// And so on.

Actually, you don't even need those if checks - multiplying by 0 produces 0, which can safely be added without affecting how much damage is dealt.

You can also split this up into a 'total damage done so far' and a 'damage per click' variable. Clicking simply increments the total damage by the damage per click, while damage per click is increased after purchasing a unit.

This can be improved further by using an array of units. At this point it's useful to create a Unit class that stores the name, cost, damage and count of a unit. This is the kind of data that you'll be modifying when you're balancing your game or when you're adding new unit types, so it helps if it's all in one place (For larger games, this sort of data is often stored in files, so game designers can modify it without the programmers having to recompile the game.):

units = new Unit[] {
    // Unit(string name, string image, BigInteger cost, BigInteger damage)
    new Unit("knife", "res/a.knife.png", TEN, ONE),
    new Unit("pistol", "res/b.pistol.png", HUNDRED, TEN),
    // And so on
}

With an array or list of units, calculating damage can be done with a simple for loop. The same goes for creating unit purchase buttons and labels - a lot of repetitive code can be simplified there:

for (Unit unit : units)
{
    // The code here is executed once for each unit in the units list,
    // so you can create a label and purchase button for each unit here.

    // Also, rather than duplicating cost and damage in html strings,
    // refer to unit properties instead (unit.cost, unit.damage, and so on)
    // to build these html strings.
}

Other notes

Variable names should be descriptive, they should help you (and others) understand what the code does. biUnit1 isn't very clear. unit1Count or numberOfUnit1Purchased would be better.

Type prefixes (hungarian notation) are redundant. The declaration already tells you what type something is, and a modern-day IDE w

StackExchange Code Review Q#126634, answer score: 2