Compacting code for World's Hardest Game remake

I was wondering if someone could quickly skim over this for me really quickly and tell me if there's anything I could improve code-wise. What I have right now seems pretty big and clustered, and I'm almost positive that there's a way it can be compressed and made more efficient, and maybe even faster. I'm also hopeful for a way to remove the threads in my code if possible. Thanks for the help :P

By the way, this game is an attempted remake of The World's Hardest Game by Snubby Land and Armor Games, and it can be played here: http://armorgames.com/play/1043/the-worlds-hardest-game

Game.java

```
package net.thedanpage.worldshardestgame;

import java.awt.BasicStroke;
import java.awt.Color;
import java.awt.Dialog;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.FontMetrics;
import java.awt.GradientPaint;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Image;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.font.FontRenderContext;
import java.awt.font.TextLayout;
import java.awt.geom.AffineTransform;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.logging.Level;
import java.util.logging.Logger;

import javax.swing.ImageIcon;
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.Timer;

import kuusisto.tinysound.Music;
import kuusisto.tinysound.Sound;
import kuusisto.tinysound.TinySound;

public class Game extends JPanel implements ActionListener {

/** An instance of the game. */
private static Game game;

/** The timer used for the game's clock. */
private Timer t = new Timer(5, this);

/** Used for logging information during the game. */
public final static Logger logger = Logger.getLogger(Game.class.ge

try-with-resources

static BufferedWriter bw = null;

You make this a class field. So it is common to every object of this class. Why?

public static void appendToFile(String filepath, String s) {
        try {
            // APPEND MODE SET HERE
            bw = new BufferedWriter(new FileWriter(filepath, true));
            bw.write(s);
            bw.newLine();
            bw.flush();
        } catch (IOException e) {
            System.out.println(e);
        } finally { // always close the file
            if (bw != null)
                try {
                    bw.close();
                } catch (IOException ioe2) {
                    // just ignore it
                }
        } // end try/catch/finally
    }

One would think that this would check if bw is already instantiated before operating on bw. But it doesn't. It writes over it and keeps going. If it doesn't care about previous values of bw, why save them? This holds a reference to a closed BufferedWriter until the next time it needs one. Then it creates a new object and sends the old one for garbage collection. Why wait? Why not just send the old object for garbage collection as soon as you are done using it?

Why not just replace the whole thing with

public static void appendToFile(String filepath, String s) {
        // APPEND MODE SET on the FileWriter
        try (BufferedWriter bw = new BufferedWriter(new FileWriter(filepath, true))) {
            bw.write(s);
            bw.newLine();
            bw.flush();
        } catch (IOException e) {
            System.out.println(e);
        }
    }

Now the try-with-resources handles closing the file. No manual finally block needed. That alone cuts almost half the code of the method. This also limits the scope to just the four places it is used.

Since we always create a new BufferedWriter, this just makes it a local variable. No class field needed.

Also, you may want to change the comment to explain why append mode is set on the FileWriter rather than simply saying that append mode is set. We can see that the FileWriter object is created in append mode if we read the JavaDoc on calling it with a Boolean second argument. What I still don't know though is why that matters. I'd much prefer to see

// append to FileWriter so that ...

With the ... replaced with a real explanation.

As is, I can see that you found this important but I have no idea why. The current comment tells me nothing that the code doesn't. I would expect a method called appendToFile to use append mode. In fact, it leaves me a bit confused, as I don't know which statement is supposed to set append mode until I read the code.

Don't Repeat Yourself (DRY)

void checkCollisionUp(GameLevel level) {
        if (getRelativeTile(level, this.x - 14, this.y + 24, 0, -1) != null &&
                getRelativeTile(level, this.x - 14, this.y + 24, 0, -1).getType() == 0 ||
                getRelativeTile(level, this.x + 15, this.y + 24, 0, -1) != null &&
                getRelativeTile(level, this.x + 15, this.y + 24, 0, -1).getType() == 0) {
            this.collidingUp = true;
            return;
        }
        this.collidingUp = false;
    }

You have four of these methods. All of them have the same basic pattern. You may want to consider @SimonForsberg's Direction enum pattern for this. But even without that, you could make this code considerably simpler.

void checkCollisionUp(GameLevel level) {
        collidingUp = hasCollision(level, x - 14, y + 24, x + 15, y + 24, 0, -1);
    }

I prefer not using the this. unless needed to disambiguate. That of course is up to you. It works both ways. I'm just pointing out that I don't use it.

Then of course you need

bool hasCollision(GameLevel level, int x1, int y1, int x2, int y2, int xOff, int yOff) {
        Tile relative1 = getRelativeTile(level, x1, y1, xOff, yOff);
        Tile relative2 = getRelativeTile(level, x2, y2, xOff, yOff);

        return relative1 != null && relative1.getType() == 0
            || relative2 != null && relative2.getType() == 0;
    }

Now there are five simple methods rather than four complex and repetitive methods.

I prefer to put operators at the beginning of the lines rather than at the end. This makes it easier to scan down the left side and get all the critical information.

I don't like the numbered variables, but I don't have better names for them. Perhaps topLeft and bottomRight or small and big or close and far.

This only calls each method once instead of calling both twice.

I'm not crazy about all the -14, +15, and +24 in that code. I would rather those were defined as constants in some way. I note that while it is -14 and +15 three out of four times, the other time it is -15 and +15. Is that deliberate? Or a mistake? I haven't tried to investigate it enough to tell.

If getRelativeTile we

StackExchange Code Review Q#131192, answer score: 3