Re: Create grid
On 1/18/2010 11:42 AM, Fencer wrote:
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.
--
Eric Sosman
esosman@ieee-dot-org.invalid