HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Making a game called Hack_exe

Submitted by: @import:stackexchange-codereview··
0
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

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 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.