Optimising Code an Array of Strings for a History in Java

Question

I am seeking guidance in the respect of optimising code. The code I have written is for a text-based game in which you type in commands into a command bar. One feature I wished to incorporate into my interface was the ability to scroll through a history of one's last 100 commands entered using the up and down arrow keys so that it would be more convenient for the user to play the game.

I have designed a class in which uses a String[] that will store each new entry in the second position (Array[1]) and move all entries back one position while the first position of the array (Array[0]) is just a blank, empty string. The code initialises the array to have 101 values to compensate for the first position being a blank line.

When a user inputs 0 - 100 in that order, it should then give me the reverse of the order (almost like a last in, first out kind of situation, but storing the last 100 values as opposed to removing them once they are accessed), and since 0 - 100 is 101 values, the last value will be overwritten.

Thus, scrolling up through the history, it would give me 100, 99, 98, ..., 2, 1. If I were to select 50 from the list, it would then be 50, 100, 99, ..., 3, 2. The code indeed does this.

The code is listed below:

public class CommandHistory {
private String[] history;
private final int firstIndex = 1;
private static int currentIndex = 0;

/**
 * Default constructor, stores last 100 entries of commands plus the blank
 * entry at the first index
 */
public CommandHistory() {
    history = new String[101];
}

/**
 * Constructor with a capacity, stores the last (capacity) entries of
 * commands plus the blank entry at the first index
 * 
 * @param capacity
 *            Capacity of the commands history list
 */
public CommandHistory(int capacity) {
    history = new String[capacity + 1];
}

/**
 * Returns the size (length) of the history list
 * 
 * @return The size (length) of the history list
 */
private int size() {
    return history.length;
}

/**
 * Adds a command to the command history log
 * 
 * @param command
 *            Command to be added to the history log
 */
public void add(String command) {
    history[0] = "";
    if (!command.equals("")) {
        for (int i = firstIndex; i < size();) {
            if (history[i] == null) {
                history[i] = command;
                break;
            } else {
                for (int j = size() - 1; j > firstIndex; j--) {
                    history[j] = history[j - 1];
                }
                history[firstIndex] = command;
                break;
            }
        }
        currentIndex = 0;
    }
}

/**
 * Gets the previous command in the history list
 * 
 * @return The previous command from the history list
 */
public String previous() {
    if (currentIndex > 0) {
        currentIndex--;
    }
    return history[currentIndex];
}

/**
 * Gets the next command in the history list
 * 
 * @return The next command from the history list
 */
public String next() {
    if (currentIndex >= 0 && (history[currentIndex + 1] != null)) {
        currentIndex++;
    }
    return history[currentIndex];
}

/**
 * Clears the command history list
 */
public void clear() {
    for (int i = firstIndex; i < size(); i++) {
        history[i] = null;
    }
    currentIndex = 0;
}

/**
 * Returns the entire command history log
 */
public String toString() {
    String history = "";
    for (int i = 0; i < size(); i++) {
        history += this.history[i];
    }
    return history;
}
}

In my interface class, once the user types something into the command bar and hits enter, it will get the text currently stored in the bar, uses the add method to add it to the history, parses the command via another class, and then sets the text in the bar to blank.

Pressing the up arrow calls the next method which scrolls up the list, and the down arrow calls the previous method which scrolls down the list.

It seems to work in every way I wish it to, but I was wondering if there was some way to optimise this code or perhaps even code it in a completely different way. I am making this game to keep myself practiced in Java and also to learn new and more advanced things, so I'd love to hear any suggestions on how to do so.

Answer 1

The comments to your question have already pointed out that you are somehow trying to reinvent the wheel by implementing functionality that the standard Java class library already provides to some extent (see LinkedList/Queue and Arraylist). But since you say you want to keep yourself practiced in Java I guess it is perfectly fine if you try to implement your own command history from scratch.

Here are some of my observations/suggestions:

1) It is not necessary and very counter-intuitive to declare a final first index of 1. It would be easy to start with a default index of 0 and add corresponding checks where necessary.

2) Forget about your private size() method - it is just returning the length of the internal array anyway (i.e. the initial capacity+1). Instead consider adding a public size() method that returns the actual number of added commands and internally update the actual size when adding new commands (see e.g. java.util.ArrayList for reference).

3) At the moment every call to add(String command) will set history[0] = "", which is not necessary. If you want the first index to be "", set it in the constructor. This is also a clear sign, that it would perhaps be better to start with an initial index of 0 instead of 1.

4) A minor issue: "if (!command.equals(""))" during your add method is perhaps OK for such a specialized class but it should definitely be commented in the documentation of the method. Personally I would always let the calling class decide if an empty "" command is considered valid or not. Also this method will throw an undocumented NullPointerException, when null is used as an argument. Consider changing this to "if (!"".equals(command))" or throw an IllegalArgumentException if null is added.

5) "if (history[i] == null)" during the add method is completely unnecessary, if you internally keep a pointer to the actual size of the commands - this is actually a special case that will only be true, when the very first command is added to the command history (i.e. when it's actual size == 0).

6) Having two nested for loops in your add method implementation is also unnecessary, if you keep a pointer to the actual size (see example below)

7) I would reconsider if it is necessary to keep a pointer to the current index in the command history. Personally I would avoid storing such a pointer and leave these details to the calling class - i.e. remove the previous and next methods and either provide a forward/backward Iterator and/or a random access to the index of the available commands. Interestingly, when this functionality is removed from your command history class, it actually comes down to either an implementation of a LinkedList or an ArrayList- whichever way you go. So in the end using one of the built in Java collections would actually be the way to go.

8) Last but nor least I would reconsider if it is useful to insert added commands at the beginning of the list - I believe it would be more natural to append them to the end as e.g. ArrayList does. Adding the commands to the end would make the swapping of all current commands during each call to add() unnecessary...

Here are some of the suggested changes to your class (not really tested...)

public class CommandHistory {
private String[] history;
private int size;
private static int currentIndex = 0;

/**
 * Default constructor, stores last 100 entries of commands plus the blank
 * entry at the first index
 */
public CommandHistory() {
    this(100);
}

/**
 * Constructor with a capacity, stores the last (capacity) entries of
 * commands plus the blank entry at the first index
 * 
 * @param capacity
 *            Capacity of the commands history list
 */
public CommandHistory(int capacity) {
    history = new String[capacity];
}

/**
 * Returns the size (length) of the history list
 * 
 * @return The size (length) of the history list
 */
public int size() {
    return size;
}

/**
 * Adds a command to the command history log
 * 
 * @param command
 *            Command to be added to the history log
 */
public void add(String command) {
    if (!"".equals(command)) {
        if (this.size < history.length) {
            this.size++;
        }
        for (int i = size-1; i >0; i--) {
            history[i] = history[i-1];
        }
        history[0] = command;
        currentIndex = 0;
    }
}

/**
 * Gets the previous command in the history list
 * 
 * @return The previous command from the history list
 */
public String previous() {
    if (currentIndex >= 0 && currentIndex < size-1) {
        currentIndex++;
    }
    return history[currentIndex];
}

/**
 * Gets the next command in the history list
 * 
 * @return The next command from the history list
 */
public String next() {
    if (currentIndex > 0 && currentIndex < size) {
        currentIndex--;
    }
    return history[currentIndex];
}

/**
 * Clears the command history list
 */
public void clear() {
    for (int i = 0; i < size; i++) {
        history[i] = null;
    }
    currentIndex = 0;
}

/**
 * Returns the entire command history log
 */
public String toString() {
    String history = "";
    for (int i = 0; i < size; i++) {
        history += this.history[i] + ", ";
    }
    return history;
}

}

Well, I guess I have invested far too much time for this, but I learned quite a bit myself on the way - so thanks ;-) Hope some of this is useful for you.