patternjavaMinor
Making a game called Hack_exe
Viewed 0 times
hack_exemakinggamecalled
Problem
I have been making a small game which resembles a computers terminal. There isn't much in the way of a game, but before I go any further I want to know if there are any things that I can improve, or make more efficient.
```
import java.util.ArrayList;
import java.util.Scanner;
import java.util.Date;
import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.io.*;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import javax.swing.AbstractAction;
import javax.swing.Action;
import javax.swing.BoxLayout;
import javax.swing.JFrame;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import javax.swing.text.DefaultCaret;
@SuppressWarnings("serial")
public class Main_Menu extends JFrame{
static volatile Boolean hasInput = false;
static String inputEarly = "";
static JTextArea textWindow = new JTextArea();
static JTextField textInput = new JTextField();
static JFrame frame = new JFrame("Hack_exe");
static BoxLayout boxLayout = new BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS);
static JScrollPane scrollPane = new JScrollPane(textWindow);
static Action action = new AbstractAction(){
@Override
public void actionPerformed(ActionEvent e) {
textWindow.append(textInput.getText());
inputEarly = textInput.getText();
hasInput = true;
textInput.setText("");
}
};
DefaultCaret caret = (DefaultCaret)textWindow.getCaret();
@SuppressWarnings("unused")
private static Scanner in;
public static String typePhrase(String phrase) {
textInput.setEditable(false);
for(int i = 0; i openable = new ArrayList<>();
ArrayList commands = new ArrayList<>();
ArrayList mail = new ArrayList<>();
ArrayList mailCommands = new ArrayList<>();
ArrayList previousCommands = new ArrayList<>();
openable.add("Welcome");
commands.add("ls - Lists al
```
import java.util.ArrayList;
import java.util.Scanner;
import java.util.Date;
import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.io.*;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import javax.swing.AbstractAction;
import javax.swing.Action;
import javax.swing.BoxLayout;
import javax.swing.JFrame;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import javax.swing.text.DefaultCaret;
@SuppressWarnings("serial")
public class Main_Menu extends JFrame{
static volatile Boolean hasInput = false;
static String inputEarly = "";
static JTextArea textWindow = new JTextArea();
static JTextField textInput = new JTextField();
static JFrame frame = new JFrame("Hack_exe");
static BoxLayout boxLayout = new BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS);
static JScrollPane scrollPane = new JScrollPane(textWindow);
static Action action = new AbstractAction(){
@Override
public void actionPerformed(ActionEvent e) {
textWindow.append(textInput.getText());
inputEarly = textInput.getText();
hasInput = true;
textInput.setText("");
}
};
DefaultCaret caret = (DefaultCaret)textWindow.getCaret();
@SuppressWarnings("unused")
private static Scanner in;
public static String typePhrase(String phrase) {
textInput.setEditable(false);
for(int i = 0; i openable = new ArrayList<>();
ArrayList commands = new ArrayList<>();
ArrayList mail = new ArrayList<>();
ArrayList mailCommands = new ArrayList<>();
ArrayList previousCommands = new ArrayList<>();
openable.add("Welcome");
commands.add("ls - Lists al
Solution
I'd recomment to fix the warnings instead of suppressing them in first place.
Concerning performance, you don't need to create a new
To avoid the large if/else block at the bottom, refactor this structure using a command pattern. Your input handling can therefore be refactored to a class
And I highly recommend to start documenting your application early. If you stop your project and return a couple of weeks later you will thank yourself if you added some documentation so you know what which methods is (or should) doing.
Last but not least, avoid
Update:
Througout your code you have a loop that waits for input:
which basically should wait for input and proceeds after input was found. In terms of performance this will cause a thread to work at full capacity. If you have multiple threads that will wait for input, this will drastically slow down your application. To avoid that you can use
On analyzing your code further I found this gem:
First, remove the empty else block if you don't need it. Why do you allow to add an empty
Refactor equal code lines to a method and invoke the method instead of keeping the duplicate code like your wait for input or previous command handling.
Concerning performance, you don't need to create a new
SimpleDateFormat every time you want to create a new getDateTime() response, so refactor it to a private field.typePhrase(...) and typeLoad(...) are very similar and therefore are a good point for refactoring. You could f.e. pass the increment step through the parameter. As both return only a blank character you can simply replace this with changing the return type to void. Performancewise just printing the whole text at once is probably better then iterating and sleeping for 50ms per/every 9th character.To avoid the large if/else block at the bottom, refactor this structure using a command pattern. Your input handling can therefore be refactored to a class
Console or the like which keeps track of registered commands and iterates on an input-event through all registered commands and on a match executes the respective command logic. While this may not yield a noticable speed improvement (but at this time it is way to early to (over-)optimize your application), it makes your code much more readable and therefore understandable. Also, through the use of patterns you may reduce the documentation overhead as patterns depict common knowledge.And I highly recommend to start documenting your application early. If you stop your project and return a couple of weeks later you will thank yourself if you added some documentation so you know what which methods is (or should) doing.
Last but not least, avoid
while(true) { ... } constructs if possible.Update:
Througout your code you have a loop that waits for input:
while(true) {
if(hasInput) {
input = inputEarly;
hasInput = false;
break;
}
}which basically should wait for input and proceeds after input was found. In terms of performance this will cause a thread to work at full capacity. If you have multiple threads that will wait for input, this will drastically slow down your application. To avoid that you can use
wait() and notify() construct or better its replacement located in java.util.concurrent package Lock. The input itself should be checked in an own IO handler thread to gain the possibility to react on input events even though the rendering takes place.On analyzing your code further I found this gem:
previousCommands.add(input);
if(previousCommands.size() > 5) {
previousCommands.remove(0);
} else {
}
for(int i = 0; i < previousCommands.size(); i++) {
if(previousCommands.get(i).equals("")) {
previousCommands.remove(i);
}
}First, remove the empty else block if you don't need it. Why do you allow to add an empty
previousCommand if you later on remove it anyway? Just don't add it in first place so you can avoid the latter iteration which might not work as removing entries while iterating is not possible to my knowledge unsless you explicitely use an iterator and invoke the remove on the iterator object.Refactor equal code lines to a method and invoke the method instead of keeping the duplicate code like your wait for input or previous command handling.
Code Snippets
while(true) {
if(hasInput) {
input = inputEarly;
hasInput = false;
break;
}
}previousCommands.add(input);
if(previousCommands.size() > 5) {
previousCommands.remove(0);
} else {
}
for(int i = 0; i < previousCommands.size(); i++) {
if(previousCommands.get(i).equals("")) {
previousCommands.remove(i);
}
}Context
StackExchange Code Review Q#135610, answer score: 2
Revisions (0)
No revisions yet.