Method that receives a hexadecimal string and returns a formatted matrix of those values

Question

This method is part of a larger project and comes after encoding ascii characters into hexadecimal values, so it assumes the parameter is always a valid hex string.

public static String toMatrix(String hex) {
 StringBuilder builder = new StringBuilder();
 int rows = 0; 
 int columns = 16;
 int row = 0;
 int column = 0;
 rows = ( hex.length() / 32 ) + ( hex.length() % 32 );
 for (row = 0; row < rows; row++) {
 for (column = 0; column < columns; column++) {
 if (hex.length() > 0) {
 builder.append(hex.substring(0,2));
 hex = hex.substring(2,hex.length());
 if ((column + 1) % 4 == 0) {
 builder.append(" "); 
 }else {
 builder.append(" ");
 }
 }
 }
 builder.append("\n");
 column = 0;
 }
 return builder.toString().trim();
}

Input:

1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1C1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D1D

Output:

1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 
1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 1C 
1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 
1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D 1D

In which way can this method be better?

Answer 1

This function has a few inefficiencies:

• Calling hex.substring(2, hex.length()) makes a fresh copy of hex, copying the entire contents except the first two characters. (Before Java 7, there used to be an optimization for .substring().) This is very bad, since it turns an algorithm that should be O(n) into O(n²).
• Similarly, calling .trim() at the end copies nearly the entire string, just to drop the trailing whitespace. It would be more efficient to truncate builder by calling builder.setLength(...) before .toString(). Better yet, design your loop so that it doesn't append superfluous whitespace at the end.
• The StringBuilder has to guess how large its buffer needs to be, and if it underestimates, it will need to reallocate and copy the buffer. We know that for every eight characters of input, there will be approximately 13 characters of output (overestimating slightly).

Calculating rows and columns overcomplicates the problem. Every 16th delimiter will be a newline. Every fourth delimiter will be a double space. All the others will be a single space. Except, of course, you don't prepend anything at the very beginning.

public static String toMatrix(String hex) {
 StringBuilder builder = new StringBuilder(hex.length() * 13 / 8);
 String delim = "";
 for (int i = 0; i + 1 < hex.length(); i += 2) {
 builder.append(delim).append(hex.substring(i, i + 2));
 delim = (i % 32 == 30) ? "\n" :
 (i % 8 == 6) ? " " : " ";
 }
 return builder.toString();
}

• enon: This is the explanation I was looking for. Thank you very much.

Answer 2

 int columns = 16;

This should be a constant and defined outside the method.

public static final int MATRIX_WIDTH = 32;

I prefer singular names for scalar variables. I use plural names for collections.

I'll explain the value change later.

 int row = 0;
 int column = 0;

These could be declared with their loops.

 rows = ( hex.length() / 32 ) + ( hex.length() % 32 );

This seems wrong. Consider

 matrixHeight = ( hex.length() / MATRIX_WIDTH ) + (( hex.length() % MATRIX_WIDTH == 0 ) ? 0 : 1);

Does that do what you want? Test it with a 224 character string to be sure.

But you could probably do without it.

 for (row = 0; row < rows; row++) {
 for (column = 0; column < columns; column++) {
 if (hex.length() > 0) {

I'd try

 while ( !hex.isEmpty() ) {
 for (int column = 2; column <= MATRIX_WIDTH && !hex.isEmpty(); column += 2) {

This way you don't keep looping after reaching the end of the string. And you don't need row or rows at all.

I'll explain the 2 in a moment.

 hex = hex.substring(2,hex.length());

This could be just

 hex = hex.substring(2);

You don't need to specify the end of the string if that's how far you want to go. It's the default in the one argument version.

 if ((column + 1) % 4 == 0) {

But if we start at 2, this could be just

 if (column % 8 == 0) {

Now you don't have to calculate every iteration. You can just work off the variable directly.