Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Naming:

Naming:

You're naming your things terribly:

JTextField t1, t2, t3, t4;
JLabel j4;
ButtonListener bl1;
ButtonListener2 bl2;
ButtonListener3 bl3;

Better would definitely be:

JTextField guessInput, bestScore, guessText; 

You should already be able to see: t4 was unused, guessInput (taken from the comments at initialization) and guessText (also take from the comment at init) are somewhat doubled.

ButtonListener, ButtonListener2, ButtonListener3

Wait what??? you have 3 different action listeners and call them ButtonListener 1, 2 and 3?
Shame on you. Better names would be:

GuessSubmittedListener
SurrenderListener
NewGameListener

Move this thorough your whole code.

###Starting the game:

Starting the game:

public static void main(String[] args)
{
 new Guess();
}

Conventionally you'd have something like the following instead:

public static void main(String[] args){
 Guess game = new Guess();
 game.start(); // game.run(); game.begin(); game.whatever();
}

The constructor of game is supposed to initialize it. But it never should be responsible for starting the game.

###Braces:

Braces:

The convention for Java is "egyptian braces". The opening curly brace is on the line with the block expression, the closing brace is separate. You are using the C# conventions...

###Magic Numbers:

Magic Numbers:

Your whole code is littered with magic numbers, numbers that have no explanation whatsoever. Move these numbers to constants:

int rand = (int) (Math.random()*100);

Nonononon...

private static final int RANDOM_BORDER = 101;
Random rnd = new Random();
int randNumber = rnd.nextInt(MAX_NUMBER);

Same goes for the whole initialization stuff... There's so many magic numbers there I'm almost having a breakdown right now.

###Naming:

You're naming your things terribly:

JTextField t1, t2, t3, t4;
JLabel j4;
ButtonListener bl1;
ButtonListener2 bl2;
ButtonListener3 bl3;

Better would definitely be:

JTextField guessInput, bestScore, guessText; 

You should already be able to see: t4 was unused, guessInput (taken from the comments at initialization) and guessText (also take from the comment at init) are somewhat doubled.

ButtonListener, ButtonListener2, ButtonListener3

Wait what??? you have 3 different action listeners and call them ButtonListener 1, 2 and 3?
Shame on you. Better names would be:

GuessSubmittedListener
SurrenderListener
NewGameListener

Move this thorough your whole code.

###Starting the game:

public static void main(String[] args)
{
 new Guess();
}

Conventionally you'd have something like the following instead:

public static void main(String[] args){
 Guess game = new Guess();
 game.start(); // game.run(); game.begin(); game.whatever();
}

The constructor of game is supposed to initialize it. But it never should be responsible for starting the game.

###Braces:

The convention for Java is "egyptian braces". The opening curly brace is on the line with the block expression, the closing brace is separate. You are using the C# conventions...

###Magic Numbers:

Your whole code is littered with magic numbers, numbers that have no explanation whatsoever. Move these numbers to constants:

int rand = (int) (Math.random()*100);

Nonononon...

private static final int RANDOM_BORDER = 101;
Random rnd = new Random();
int randNumber = rnd.nextInt(MAX_NUMBER);

Same goes for the whole initialization stuff... There's so many magic numbers there I'm almost having a breakdown right now.

Naming:

You're naming your things terribly:

JTextField t1, t2, t3, t4;
JLabel j4;
ButtonListener bl1;
ButtonListener2 bl2;
ButtonListener3 bl3;

Better would definitely be:

JTextField guessInput, bestScore, guessText; 

You should already be able to see: t4 was unused, guessInput (taken from the comments at initialization) and guessText (also take from the comment at init) are somewhat doubled.

ButtonListener, ButtonListener2, ButtonListener3

Wait what??? you have 3 different action listeners and call them ButtonListener 1, 2 and 3?
Shame on you. Better names would be:

GuessSubmittedListener
SurrenderListener
NewGameListener

Move this thorough your whole code.

Starting the game:

public static void main(String[] args)
{
 new Guess();
}

Conventionally you'd have something like the following instead:

public static void main(String[] args){
 Guess game = new Guess();
 game.start(); // game.run(); game.begin(); game.whatever();
}

The constructor of game is supposed to initialize it. But it never should be responsible for starting the game.

Braces:

The convention for Java is "egyptian braces". The opening curly brace is on the line with the block expression, the closing brace is separate. You are using the C# conventions...

Magic Numbers:

Your whole code is littered with magic numbers, numbers that have no explanation whatsoever. Move these numbers to constants:

int rand = (int) (Math.random()*100);

Nonononon...

private static final int RANDOM_BORDER = 101;
Random rnd = new Random();
int randNumber = rnd.nextInt(MAX_NUMBER);

Same goes for the whole initialization stuff... There's so many magic numbers there I'm almost having a breakdown right now.

Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

###Naming:

You're naming your things terribly:

JTextField t1, t2, t3, t4;
JLabel j4;
ButtonListener bl1;
ButtonListener2 bl2;
ButtonListener3 bl3;

Better would definitely be:

JTextField guessInput, bestScore, guessText; 

You should already be able to see: t4 was unused, guessInput (taken from the comments at initialization) and guessText (also take from the comment at init) are somewhat doubled.

ButtonListener, ButtonListener2, ButtonListener3

Wait what??? you have 3 different action listeners and call them ButtonListener 1, 2 and 3?
Shame on you. Better names would be:

GuessSubmittedListener
SurrenderListener
NewGameListener

Move this thorough your whole code.

###Starting the game:

public static void main(String[] args)
{
 new Guess();
}

Conventionally you'd have something like the following instead:

public static void main(String[] args){
 Guess game = new Guess();
 game.start(); // game.run(); game.begin(); game.whatever();
}

The constructor of game is supposed to initialize it. But it never should be responsible for starting the game.

###Braces:

The convention for Java is "egyptian braces". The opening curly brace is on the line with the block expression, the closing brace is separate. You are using the C# conventions...

###Magic Numbers:

Your whole code is littered with magic numbers, numbers that have no explanation whatsoever. Move these numbers to constants:

int rand = (int) (Math.random()*100);

Nonononon...

private static final int RANDOM_BORDER = 101;
Random rnd = new Random();
int randNumber = rnd.nextInt(MAX_NUMBER);

Same goes for the whole initialization stuff... There's so many magic numbers there I'm almost having a breakdown right now.

lang-java

AltStyle によって変換されたページ (->オリジナル) /