Simon Says: "Optimizations!"

A while back I asked a question about my Simon Says game and did well on my assignment! But now we need to go back in a make some modifications to the game to be more advanced. I've done that as well, but I need some tips to make it more pristine and little cleaner, some things I'm still working on, but any help here would be greatly appreciated.

Here is a ZIP file with images and a few other resources.

And GitHub if needed.

Simon.java

```
/*
*
*/
package Simon;

import java.awt.BasicStroke;
import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Font;
import java.awt.Graphics2D;
import java.awt.RenderingHints;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.ComponentAdapter;
import java.awt.event.ComponentEvent;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.UnsupportedEncodingException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Hashtable;
import java.util.Random;
import java.util.concurrent.TimeUnit;
import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.Clip;
import javax.swing.JButton;
import javax.swing.JCheckBoxMenuItem;
import javax.swing.JColorChooser;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JOptionPane;
import javax.swing.JSlider;
import javax.swing.Timer;

// TODO: Auto-generated Javadoc
/**
* The Class Simon.
*/

public class Simon implements ActionListener, MouseListener
{

/** The simon. */
public static Simon simon;

/** The renderer. */
public Renderer renderer;

/** The Constant WIDTH and HEIGHT. */
public static int WIDTH = 800;
public static int HEIGHT = 800;

/** The index pattern. */
public int flashed = 0, dar

Use interfaces when possible

ArrayList scores = new ArrayList();
ArrayList sessionScores = new ArrayList();

You should use List here instead of ArrayList when declaring the variable. It is a good idea to do this as often as you can, including when writing the arguments for methods. This will allow you to change the ArrayList to another kind of container later without changing anything else, as long as it implements the List interface.

Bad Comments

/** The simon. */
public static Simon simon;

/** The renderer. */
public Renderer renderer;

/** The Constant WIDTH and HEIGHT. */
public static int WIDTH = 800;
public static int HEIGHT = 800;

It may be a requirement for your class assignment to have these kind of comments, but they are not good. Typically the names of your variables, methods and classes should self-document what they are, rather than being defined by comments.

Generally, comments should explain Why you are doing something, rather than How you are doing it or What it is. I looked at the rest of the code, and there are absolutely no comments inside your methods, so there are probably a few places where they are missing where they could be useful.

Enums

for(int i = 0; i < panels.size(); i++) {
        int panelNumber = panels.get(i);
        if(panelNumber == 1) { 
            if(i == 0) { topLeftColor = backupTopLeftColor; }
            if(i == 1) { topLeftColor = backupTopRightColor; }
            if(i == 2) { topLeftColor = backupBottomLeftColor; }
            if(i == 3) { topLeftColor = backupBottomRightColor; }
        }
        if(panelNumber == 2) { 
            if(i == 0) { topRightColor = backupTopLeftColor; }
            if(i == 1) { topRightColor = backupTopRightColor; }
            if(i == 2) { topRightColor = backupBottomLeftColor; }
            if(i == 3) { topRightColor = backupBottomRightColor; }
        }

Any time you are using simple integers inside switch statements, or in an if clause like this one, you should change them to an enum to improve the readability of the code.

Right now, you just have a number of panels in an array, and you know in your head which position in the array the panel will be. However, no one else reading your code (or possibly using it) will know that position 2 in the array is supposed to be the top right panel. You could have an enum such as PanelPositions that could contain the types TOP_LEFT, TOP_RIGHT, etc. However, I am trying to understand this code in order to recommend to you exactly how you might use an enum in this situation, but I cannot see the best way to do it. Which leads me to my next point:

Object Oriented Code and the Single Responsibility Principle

You should not need to iterate over an array of integers like this in order to implement the game of Simon Says. The reason you have funny code like this is that your Simon class is doing way too much. It's responsible for everything, such as choosing the new pattern, drawing everything to the screen, even saving the scores to disk.

The first step to achieve a more object oriented approach here would be to completely separate the rendering of the game from the game model backing it. Perhaps the Simon class would be in charge of drawing, and there could be a SimonGame class for the game model. The Simon class would look at the SimonGame class in order to get the information that it needs to draw things to the screen. The Simon class would only be responsible for starting the SimonGame game, drawing the current state to the screen, and receiving user input and passing it to the SimonGame. Then, the SimonGame class would do things such as validating whether the player has entered the correct sequence, and generating new sequences. In this way, each class has a single responsibility, and thus it is much easier to understand what the code does, both for someone else, and for yourself when you come back to this code one day. In the same way, I would recommend moving the writing of the scores to a separate class, or maybe into the Score class.

Using this approach for the panels could mean that your SimonGame class has a List of Panel objects, rather than trying to do all of this with integers and a for loop and if statements.

StackExchange Code Review Q#113639, answer score: 10