Prev: Crash Course In Modern Hardware
Next: Best way to limit max number of characters per line in JEditorPane ?
From: Fencer on 18 Jan 2010 11:42
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
From: Eric Sosman on 18 Jan 2010 14:48
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(a)ieee-dot-org.invalid
From: Fencer on 19 Jan 2010 04:48
On 2010-01-18 20:48, Eric Sosman wrote:

>
> 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
From: John B. Matthews on 19 Jan 2010 07:37
In article <7rldg6FumcU1(a)mid.individual.net>,
Fencer <no.i.dont(a)want.mail.from.spammers.com> wrote:

> On 2010-01-18 20:48, Eric Sosman wrote:
>
> > 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.

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>

--
John B. Matthews
trashgod at gmail dot com
<http://sites.google.com/site/drjohnbmatthews>
 | 
Pages: 1
Prev: Crash Course In Modern Hardware
Next: Best way to limit max number of characters per line in JEditorPane ?