MouseListener Lag in Piano Tiles game

I was attempting to write my own simple version of the Piano Tiles game app in Java just as a fun exercise.

GameLogic.java

```
import javax.swing.*;

import java.awt.*;
import java.awt.event.*;
import java.util.*;

public class GameLogic extends JPanel
{
Tile[][] tileArray = new Tile[4][10];

int lowestIndex;
int targetTileIndex;

int counter = 0;

boolean calculating = false;

boolean gameStart = false;

public GameLogic()
{
super();
this.setPreferredSize(new Dimension(500, 400));

int tempY = 200;
int tempX = 0;
lowestIndex = 0;
targetTileIndex = 0;

for(int y = 0; y=400)
{
int tempRandom = randomNum();

for(int x = 0; x=targetX && x=targetY && y<=(targetY+100))
{
tileArray[targetTileNumber][targetTileIndex].deactivate();

if(targetTileIndex!=9)
{
targetTileIndex++;
}
else
{
targetTileIndex = 0;
}
}
else
{

}

repaint();

}

@Override
public void mouseEntered(MouseEvent arg0) {
// TODO Auto-generated method stub

}

@Override
public void mouseExited(MouseEvent arg0) {
// TODO Auto-generated method stub

}

@Override
public void mousePressed(MouseEvent arg0) {
// TODO Auto-generated method stub

}

@Override
public void mouseReleased(MouseEvent arg0) {
// TODO Auto-generated method stub

}

}

this.addMouseListener(new MyMouseListener());

}

public void paintComponent(Graphics g)
{
super.pai

Reviewing the code.

At first sight it look all nice and clean.

Nice formatting of the code.

Java standarts are a little differtent then you use, but you are consistent and that is good.

The deviation is where you set your {.
This is normally done at the same line and not a new line.

Still some improvement can be done to make your code easier to maintain.

Magic numbers

Yeah, for some people they are the hell. I'm not saying to convert all your "magic numbers" to private static final int but some you have to do.

Tile[][] tileArray = new Tile[4][10];

in combination with :

for(int y = 0; y<10; y++)
{
    int tempRandom = randomNum();
    for(int x = 0; x<4; x++)
    {
        tileArray[x][y] = new Tile(tempX, tempY);
        if(x == tempRandom)
        {
            tileArray[x][y].activate();
        }
        tempX+=65;
    }
 }

Why not making your bounds as constants and the Tile final so you can't initialize it again?

private static final int X_BOUND_TILES = 4;
private static final int Y_BOUND_TILES = 10;
private final Tile[][] tileArray = new Tile[X_BOUND_TILES][Y_BOUND_TILES];

and the for refactorred to this :

for(int y = 0; y < Y_BOUND_TILES; y++)
{
    int tempRandom = randomNum();
    for(int x = 0; x < X_BOUND_TILES; x++)
    {
        tileArray[x][y] = new Tile(tempX, tempY);
        if(x == tempRandom)
        {
            tileArray[x][y].activate();
        }
        tempX+=65;
    }
 }

This is much easier to refactor.
Just change your constant, compile and your off without having errors that could be raised if you forget to change your for loop.

Now why do you count 65 plus? What is the meaning of 65?

You use multiple time 65. When you need to change one, do they have to change all?

If the answer is yes => certainly make a constant, otherwise I suggest to refactor to a constant but this is not so important.

Scoping of variables

The default scope is package-private. All classes in the same package can access the method/field/class. Package-private is stricter than protected and public scopes, but more permissive than private scope.

Are you sure you want that scope for all your global variables?

Tile[][] tileArray = new Tile[4][10];
int lowestIndex;
int targetTileIndex;
int counter = 0;
boolean calculating = false;
boolean gameStart = false;

This should be :

private static final int X_BOUND_TILES = 4;
private static final int Y_BOUND_TILES = 10;
private final Tile[][] tileArray = new Tile[X_BOUND_TILES][Y_BOUND_TILES];
private int lowestIndex;
private int targetTileIndex;
private int counter = 0; 
private boolean calculating = false;
private boolean gameStart = false;

Default values

This is a part I have to say.

It's educated to me so I educate further but I'm not a fan of it.

private int counter = 0; 
private boolean calculating = false;
private boolean gameStart = false;

Is the same as :

private int counter; 
private boolean calculating;
private boolean gameStart;

Default value of a boolean is false.

Default value of a int is 0.

Take care, default of Integer and Boolean is null.

Defensive programming

if (lowestIndex!=9) {
    lowestIndex++;
} else {
    lowestIndex = 0;
}

We all know what you are trying to do. But what happens if lowestIndex is magically turned 10?

You have a bug that will not correct by himself.
Let's say we do the following :

if (lowestIndex<9) {
    lowestIndex++;
} else {
    lowestIndex = 0;
}

Execution is almost the same. But now we are sure when lowestIndex is changed magically the next step it will correct itself.

Further, this could be refactorred a ternary operator :

lowestIndex = lowestIndex<9?lowestIndex + 1:0;

Legacy code

You know how it's going. You write and erease code constantly.
I see in your code an else block with nothing in it.

Remove the else block cause it's not used anymore, so you get lesser and cleaner code.

Method's complexity - Separation of concerns

Most of your methods are a little long(we have seen a lot more worse then yours) and can be refactorred to smaller methods with a small specific problem.

GameLogic can be lot smaller and make 4-5 methods for it.

Example : The code of your actionlistener.

final javax.swing.Timer moveTimer = new javax.swing.Timer(1, getActionListenerForTimer());

Or something like that.

Let the method return the actionlistener, but in the creation of your actionlistener use also at least 2-3 methods.

Edit :

Your problem :

Use mousePressed instead of mouseClicked with your eventlistener.

mouseClicked looks for multiple button clicks, so it will merge some events.

StackExchange Code Review Q#51671, answer score: 4