8
\$\begingroup\$

I'm creating a ticket reservation system in Java, I want to use a Two-dimensional String array for managing the seats. I have enums that hold the plane model and the amount of seats available so the array pulls the rows and columns from there. I just want to see efficient my code is and how can I improve it.

    public String[][] createSeatArray() {
    String[] alphabet = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r"
            ,"s","t","u","v","w","x","y","z","aa","bb","cc","dd","ee","ff","gg","hh","ii","jj","kk","ll","mm","nn","oo","pp"};

    String[][] seatArray = new String[this.getNumOfRows()][this.getNumOfColumns()];
    int alphabetPos = 0;
    for(int i = 0; i < this.getNumOfRows(); i++) {
        for (int j = 1; j <= this.getNumOfColumns(); j++){
            String columnToString = String.valueOf(j);
            seatArray[i][j-1] = alphabet[alphabetPos] + columnToString;

            if (this.getNumOfColumns() == 4) {
                if (j == 2) {
                    System.out.print(seatArray[i][j-1] + "       ");
                } else {
                    System.out.print(seatArray[i][j-1] + " ");
                }
            } else {
                if (j == 3) {
                    System.out.print(seatArray[i][j-1] + "       ");
                } else {
                    System.out.print(seatArray[i][j-1] + " ");
                }
            }
        }
        System.out.println("\n");
        alphabetPos++;
    }
    return seatArray;
}

Thank you for any feedback! also keep in mind that if the indentation is off it may be due to bringing it over to stack overflow, on intellij it's perfect.

\$\endgroup\$
5
\$\begingroup\$

Here are a few optimization suggestions:

  1. You can get rid of alphabetPos because it is always equals to i.
    In fact, while you are at it, your code will be more readable if you rename i to row and j to col or column.

  2. You can also get rid of columnToString and just rely on Java's string concatenation overloads.

seatArray[i][j-1] = alphabet[alphabetPos] + j;
  1. Note that you use j multiple times as index, subtracting one, and only once as string.
    It would make your code more readable if you used zero based index, and only added 1 for the string composition.

  2. You may want to use string formatting.


for (int col = 0; col < this.getNumOfColumns(); col++) {
        seatArray[row][col] = String.format("%s%d", alphabet[row], col + 1);

        if (this.getNumOfColumns() == 4) {
            if (col == 1) {
                System.out.print(seatArray[row][col] + "       ");
            } else {
                System.out.print(seatArray[row][col] + " ");
            }
        } else {
            if (col == 2) {
                System.out.print(seatArray[row][col] + "       ");
            } else {
                System.out.print(seatArray[row][col] + " ");
            }
        }
}
  1. You can simplify your array printing by splitting it in to two parts: printing the seat number and printing the spacer.
    The first part is always the same, so no need to repeat it 4 times. The second part changes only when col and row have certain values, so one condition is enough.

System.out.print(seatArray[row][col] + " "); //this part is always printed the same

//just add a few more spaces if this is an isle seat
if ((getNumOfColumns() == 4 && col == 1) || (getNumOfColumns() != 4 && col == 2)) {
    System.out.print("      ");
}
  1. Finally, to save memory, you can get rid of the alphabet array all together, since the seat letters are sequential and rows 26 and above just have the same letter twice:

String rowStr = String.format("%c", 'a' + (row % 26));
if (row > 25) rowStr += rowStr;
seatArray[row][col] = String.format("%s%d", rowStr, col + 1);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @mdfst13 good catch, thanks! I was also missing a parenthesis on that line, fixed now. \$\endgroup\$ – Lev M. Oct 28 at 15:32
3
\$\begingroup\$

Printing/displaying the plan and building the plan are two separate concerns, so they really belong in different places. A large part of the build method complexity seems to be around the printing off the aisles between seats, which feels wrong.

You've also said that your system supports different row/column combinations based on plane model. I've certainly been on planes with two aisles between seats. Is this something your print logic will eventually need to handle?

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$
        if (this.getNumOfColumns() == 4) {
            if (j == 2) {
                System.out.print(seatArray[i][j-1] + "       ");
            } else {
                System.out.print(seatArray[i][j-1] + " ");
            }
        } else {
            if (j == 3) {
                System.out.print(seatArray[i][j-1] + "       ");
            } else {
                System.out.print(seatArray[i][j-1] + " ");
            }
        }

You can simplify this by declarations outside your loops.

    final int AISLE = (this.getNumOfColumns() == 4) ? 2 : 3;
    final StringBuilder builder = new StringBuilder();

And then replace the previous code with just

            builder.append(seatArray[i][j-1]).append(" ");
            if (j == AISLE) {
                builder.append("      ");
            }

And where your println is now, change to

        builder.append("\n");
        System.out.println(builder.toString());
        builder.setLength(0);

Or even

        builder.append("\n\n");

And after the loops, add

    System.out.print(builder.toString());

The latter version is fine so long as the plane isn't huge. If you could have a truly large number of rows, then you might be better off printing each row separately (or in groups of rows) so as not to run out of memory. But in this case, you know that you only have 42 rows at most (a to pp). Unless you are on some tiny embedded system, that should not be a problem.

StringBuilder.append is more efficient than System.out.print. So moving all the string operations into the builder and doing just one System.out.print will make the code more efficient.

By setting the AISLE constant, you save a method call on every iteration of the inner loop. And you get rid of the duplicate code.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.