Create grid

[Fencer]

Hello, I'm writing a simulator that involves the concept of a grid of
tiles. There are a number of different tile types and each tile must be
of one of those types.

I want a method that can create a new grid from a text file. Say the
file has the following content:
abb
aaa
cba
the method should create a 3x3 grid and the type of any given tile
is determined by its corresponding character in the input file.

I wanted this method to work both on real text files but also on
file-like objects (inspired by python's StringIO), so I came up with this:
package controller;

import java.io.FileReader;
import java.io.IOException;
import java.io.LineNumberReader;
import java.io.Reader;
import java.io.StringReader;

public class Controller {
public void init(final Reader reader) throws IOException {
LineNumberReader rdr = new LineNumberReader(reader);

String line = "";

while ((line = rdr.readLine()) != null) {
for (int column = 0; column < line.length(); ++column) {
int lineNum = rdr.getLineNumber() - 1;
char c = line.charAt(column);
createTile(lineNum, column, c);
}
}
}

public void createTile(int row, int column, char type) {
System.out.println(String.format("Create tile at (row, column) = " +
"(%d, %d) with type %c", row, column, type));
}

public static void main(String[] args) throws IOException {
Controller controller = new Controller();

controller.init(new StringReader("abc\ndef\nghi"));
System.out.println("");
controller.init(new FileReader("test.txt"));
}
}

Output when run:
Create tile at (row, column) = (0, 0) with type a
Create tile at (row, column) = (0, 1) with type b
Create tile at (row, column) = (0, 2) with type c
Create tile at (row, column) = (1, 0) with type d
Create tile at (row, column) = (1, 1) with type e
Create tile at (row, column) = (1, 2) with type f
Create tile at (row, column) = (2, 0) with type g
Create tile at (row, column) = (2, 1) with type h
Create tile at (row, column) = (2, 2) with type i

Create tile at (row, column) = (0, 0) with type a
Create tile at (row, column) = (0, 1) with type b
Create tile at (row, column) = (0, 2) with type c
Create tile at (row, column) = (1, 0) with type d
Create tile at (row, column) = (1, 1) with type e
Create tile at (row, column) = (1, 2) with type f
Create tile at (row, column) = (2, 0) with type g
Create tile at (row, column) = (2, 1) with type h
Create tile at (row, column) = (2, 2) with type i


This is just a test program where I check that I can read each character
(each tile) properly. It seems to work, but was this the proper way to
solve this problem?

- Fencer

 

[Eric Sosman]

Hello, I'm writing a simulator that involves the concept of a grid of
tiles. There are a number of different tile types and each tile must be
of one of those types.

I want a method that can create a new grid from a text file. Say the
file has the following content:
abb
aaa
cba
the method should create a 3x3 grid and the type of any given tile
is determined by its corresponding character in the input file.

I wanted this method to work both on real text files but also on
file-like objects (inspired by python's StringIO), so I came up with this:

The code looks reasonable (as far as it goes: You've still
not dealt with actually creating the tiles). I've interlarded
a few fairly minor comments:

package controller;

import java.io.FileReader;
import java.io.IOException;
import java.io.LineNumberReader;
import java.io.Reader;
import java.io.StringReader;

public class Controller {
public void init(final Reader reader) throws IOException {
LineNumberReader rdr = new LineNumberReader(reader);

String line = "";

Why bother to initialize `line'? Since the very first
thing you'll do with it is assign it a different value, the
as-initialized value is never used at all. Initialization
can even make things worse: For example, suppose you had
another String variable `lion' lying around, and you wrote
`lion = rdr.readLine()' by accident. If `line' were not
initialized, javac would complain if you then tried to use
the value -- but with initialization, javac won't complain
and it may take you longer to discover the error.

Initialize when the initial value might actually be
used for something; otherwise, don't.

while ((line = rdr.readLine()) != null) {

The principle of "restrict each variable to its narrowest
useful scope" says that `line' really shouldn't exist after
the end of the `while' loop. One way to do this is to create
a brand-new scope just for `line':

{
String line;
while ((line = ...) != null) {
...
}
}

Or, there's a slicker way:

for (String line; (line = ...) != null; ) {
...
}

for (int column = 0; column < line.length(); ++column) {

Some kind of sanity-checking of the length and content of
the input line would be a good idea. It is almost never a good
idea to swallow external inputs without examination. (See also
"SQL injection attack.")

int lineNum = rdr.getLineNumber() - 1;

Since `lineNum' will have the same value for every character
in the current line, it would make more sense to compute it once
before the inner loop starts than to re-compute the exact same
thing over and over again. For a three-by-three grid this won't
make any noticeable difference, but keep the "compute once, save
for later" idea in mind.

Also, dragging out all the machinery of LineNumberReader
just to get a count (a count you need to adjust, no less) seems
like overkill. Why not just use your own `int' variable, start
it at zero, and increment for each line?

char c = line.charAt(column);
createTile(lineNum, column, c);
}
}
}
[... test harness snipped ...]

Summary: Looks reasonable to me; room for small improvements.

 

[Fencer]

The code looks reasonable (as far as it goes: You've still
not dealt with actually creating the tiles). I've interlarded
a few fairly minor comments:

Thanks for your very detailed reply, Eric! I shall modify my code
accordingly. That tip about making variables go out of scope is useful.
Will make the debugging easier in methods that have a lot of local
variables in them, at least I think it's easier if the list of variables
that is presented to you by the debugger is short rather than long.

- Fencer

 

[John B. Matthews]

Thanks for your very detailed reply, Eric! I shall modify my code
accordingly. That tip about making variables go out of scope is
useful. Will make the debugging easier in methods that have a lot of
local variables in them, at least I think it's easier if the list of
variables that is presented to you by the debugger is short rather
than long.

You might also look at some common creational patterns, such as Factory
and Builder:

<http://en.wikipedia.org/wiki/Factory_method_pattern>
<http://en.wikipedia.org/wiki/Builder_pattern>

This article by Joshua Bloch discusses both:

<http://www.ddj.com/java/208403883>