Improving this TileMap

Question

I created a TileMap-class to load tile sets, create and display tile maps. It is far from perfect so I want your opinions on how to improve it!

How it works:

1. Create TileMap object with "new TileMap( int tilesize )".
2. Create a tile set with "createSet( String path )" and specify the tile types with "setTileSet( int row, int col, Integer type )". This tile get's saved in a HashMap to later access it by it's type-variable.
3. Create a tile map from a file with "createMap( String path )". I want to add in another method to create an empty tile map which you then can edit with "setTileMap( int row, int col, Integer type )" and output this map as a file to later load it (got it working but it's buggy).

I created a second class Map which you can use to output a map file. Please also take a look at that. (Problem with that: I made a simple method in TileMap called write(path, Tile[][]) which created a Map object and then outputted it but it was buggy. I loaded a map and let the same map be written by this method but it came out differently. Rows and columns were exchange and the whole map were turned 90°. Appreciate your help on that one too).

The Code: I got rid of all JavaDoc since I did my best to explain it above. The code also isn't done so you can find some of my annotations about future ideas in there.

TileMap class:

// Settings
private int tilesize;
// TileSet
private BufferedImage sourceImage;
private HashMap<Integer, Tile> tileList;
// TileMap
private Tile[][] tileMap;
private int mapRows;
private int mapCols;
public TileMap( int tilesize ) {
 this.tilesize = tilesize;
}
public void createSet( String path ) throws IOException {
 sourceImage = ImageIO.read( getClass().getResourceAsStream( path ) );
 tileList = new HashMap<Integer, Tile>();
}
public void setTileSet( int row, int col, Integer type ) {
 BufferedImage subImgae = sourceImage.getSubimage( row*tilesize, col*tilesize, tilesize, tilesize );
 tileList.put( type, new Tile( subImgae, type ) );
}
public Tile getSetTile( Integer type ) {
 return tileList.get( type );
}
public void createMap( String path ) throws NumberFormatException, IOException {
 InputStream in = getClass().getResourceAsStream( path );
 BufferedReader reader = new BufferedReader( new InputStreamReader( in ) );
 mapCols = Integer.parseInt( reader.readLine() );
 mapRows = Integer.parseInt( reader.readLine() );
 tileMap = new Tile[mapRows][mapCols];
 for( int row = 0; row < mapRows; row++ ) {
 String line = reader.readLine();
 String[] tokens = line.split( " " );
 for( int col = 0; col < mapCols; col++ ) {
 tileMap[row][col] = getSetTile( Integer.parseInt( tokens[col] ) ); 
 }
 }
 in.close();
 reader.close();
}
public void setTileMap( int row, int col, Integer type ) {
 tileMap[row][col] = tileList.get( type );
}
public void drawMap( int x, int y, int width, int height, java.awt.Graphics2D g ) {
 for( int row = 0; row < mapRows; row++ ) {
 for( int col = 0; col < mapCols; col++ ) {
 g.drawImage( tileMap[row][col].getImage(), x + col * tilesize, y + row * tilesize, null );
 }
 } // add out-of-bound-checking
}

Map class:

private Tile[][] tileList;
private int rows;
private int cols;
public Map( int rows, int cols, Tile[][] tiles ) {
 this.rows = rows;
 this.cols = cols;
 this.tileList = tiles;
}
public void write( String path ) throws FileNotFoundException {
 PrintWriter writer = new PrintWriter( path );
 // Write Map Size
 writer.println( rows );
 writer.println( cols );
 // Write bytes
 for (int col = 0; col < cols; col++) {
 for (int row = 0; row < rows; row++) {
 writer.print( tileList[row][col].getType() + " " );
 }
 writer.println( "" );
 }
 System.out.println( "Map written to " + path );
 writer.close();
}

To quickly add something: That's my first ever approach on a tile map and I did it without looking up many tutorials since they all looked confusing to me. Now that I ruffly know how a tile map works I understand them better but still not enough to get along without help - your help :)

• t3chb0t: What should I do when someone answers my question?
• Roovy: How does this code help anybody if it's wrong? I see the point but... mäh. Okay...
• t3chb0t: It helps a lot because people can see the before and after effects. They can clearly see what was wrong and how it should be improved. Not every review quotes the original code so there is no way to say whether the reviewer was right and what he actually did.

Answer 1

Please choose better names

Don't "reuse" standard lib class names

You named one of your classes Mapbut there is already a class in the JVM having that name (the Map interface)

Don't confuse your readers

• tileMap your central variable is called tileMap but it is an array.

  I understand, that the term map comes from your business case where the term map relates to a 2 dimensional rasterized representation of a geographical plane. But programmers understand by the term map a collection type that assignes values to keys.

  So you should find a better name for tileMap e.g. simply tiles or gameField or alike...

• setTileMap
  You have a method called setTileMap. This is what programmers call a setter method. It is expected to put the value passed as parameter directly into a member variable of the class.

  But in your case it behaves differently.

  This method should better be named setTileAtPosition()

• Roovy: Thank you! Very informative. Map really just was a place holder since I needed something to quickly create a map file.
• Roovy: Do you can help me with this: As I mentionen, if you output a map via the Map class, then read it with createMap() and then outputting it again through creating a Map object (as I did in the here left out method writeMap(), why is the map turned 90°? In the first file its like 1121 from left to right but in the second file it's 1121 from top to bottom? (Hope it's understandable, if not I will poste a example)
• Timothy Truckle: "why is the map turned 90°?" This side is for reviews, not for bugfixing. Please ask bug related questions at stackoverflow.com
• Timothy Truckle: "why is the map turned 90°?" this is because you iterate over rows and columns the other way around in Map
• Roovy: "This side is for reviews, not for bug fixing."i I know, but asking the same question as well as the question about thoughts to this tile map I got redirected here - "A place to work on code together"

Answer 2

Good Style

Good, consistent style accross the files. Excellent spacing, very easy to read. Top notch!

Now I'll get meaner, sorry!

---

Missing bits

Please include the complete class declaration in your files:

public class MyCLass { // <-- This crucial bit is missing.
 // --> You only provided what is in here <---
}

This way I can be sure the class is not extending something which would hide some code away. Also, Map could have been a static class of TileMap or something. I'll assume they are distinct, vanilla classes.

Also we're missing the Tile.java.

Finally, don't remove Javadoc. I don't think it's reduncant to the code, because I had trouble seeing the difference between your two classes' role at first. Also there's lots to say about a Javadoc. The absence of Javadoc is bad, but there is also such a thing as too much (clutter) Javadoc, and a badly-written Javadoc.

---

Naming is important

I won't repeat @TimothyTruckle's answer here. It's sound. Here's my additional notes:

1. I suggest you see bigger with your naming.

Eventually these classes will incorporate more functionalities. Maybe TileMap --> TileWorld or GameWorld?

2. Map is not only clashing with the JDK, it's also not very informing.

What does the class do? It is exporting the tile data to a file. Clearly Map doesn't explain that.

Now normally we don't call a class by what it does, but by what it is. So then if it's not a Map maybe it's an Exporter or a FileSaver? Let's see bigger and call it GameSaver with a .saveTilesToFile(String filePath) method and maybe later we can add .saveCharactersToFile(String filePath) etc to make the class richer.

3. TileMap.createMap(String path) should be named createFromFile, but I would really like it to be static and perform the whole job, correctly:

   public static TileMap createFromFile(String filePath){ int tilesize = // (...) Read tilesize in file TileMap toReturn = new TileMap(tilesize); // (...) Initialize everything from the file using toReturn instead of this return toReturn; }

Or maybe put it in your GameSaver as

public static GameWorld GameSaver.loadGame(String filePath)

Because I sure as hell don't know in which order to call all these setup methods! Not right away at least. This hints at maybe rovinding a rich, stateful Builder to this class?

4. Finally, getSetTile should be named getFromTileSet(int tileType), but this one isn't even needed, as we'll se later.

---

TileMap is leaking

1. TileMap is using an Integer to represent a Tile code.
   While it is fine to use int to store data in a file, it is not very helpful in code to manipulate the number 5 while thinking "This means grass". The Integer encoding should only show up in file loading/writing functions. Try to keep your Map<Integer, Tile> in the GameSaver object. Also you could name it encoding, since this is what it represents. You would probably need a Map<Tile, Integer> decoding as well, but since Two Tile can be of the same type, yet be different instances, this is a problem. In the end you're looking for a BiMap.
2. TileMap is using a Tile to represent Tile type. This is not good, first and foremost because you'll be using lots of Tile instances, many of which will share the same Type. So comparing tile1.equals(tile2) will compare tiles, not tile types. You have no way of comparing tile types.
   Therefore you need a TileType Object. The added value is that while drawing a bunch of tiles of type Grass, you'll be able to use various images of grass of your tileset (to reduce repeating pattern) while handling all their types as simply 'grass' for game mechanics.

With this, you won't need getSetTile at all.

---

Don't repeat state

Map is storing internally a representation of the tiled world. Is that its function? No, that is TileMap (or TileWorld) 's job. What if TileWorld wants to switch x/y, or store the data in a sparse array, or with blocs which are generated on the fly? then Map has to adapt its own representation. You failed to implement Encapsulation and you've opened the gates of maintenance hell.

Thankfully it's easy to cure. Just replace Map's tileList, rows and cols variables with a single private GameWorld world; field.

Map can simply store in the file the data it can access using your beautifully crafted GameWorld.getTileAt(int x, int y) method. Oh, wait...

---

Your TileSet classes are unsuable

You can create one using setTileSet, createSet etc, but the only Getter method you provided was getSetTile. How do I know if I have grass or sea at tile (5, 10) ? You need to provide a Tile getTileAt(int x, int y) method. It should include bound checking etc.

I would suggest adding Tile[][] getRectArea(int x, int y, int width, int height) to optimize drawing or loading of an area. This lets the GameWorld optimize its data-gathering (in case of blocs, and to minimize bound-checking) and the MapPainter optimize its painting (using local rectangle area).

---

Heard of Java 6 ?

It provides a try-with-resources statement which would make your code much more robust. Like this from TileMap.createMap()

try( InputStream in = getClass().getResourceAsStream( path );
 BufferedReader reader = new BufferedReader( new InputStreamReader( in ) );
 ) {
 mapCols = Integer.parseInt( reader.readLine() );
 mapRows = Integer.parseInt( reader.readLine() );
 tileMap = new Tile[mapRows][mapCols];
 for( int row = 0; row < mapRows; row++ ) {
 String line = reader.readLine();
 String[] tokens = line.split( " " );
 for( int col = 0; col < mapCols; col++ ) {
 tileMap[row][col] = getSetTile( Integer.parseInt( tokens[col] ) ); 
 }
 }
} // No need to manually close stuff!

---

Split responsibilities

Each object should mind their own business. I see an infraction to this rule coming in. It's here:

TileMap class:

// Settings private int tilesize;

This is not used anywhere. So it does not represent what is in the world. I suspect it represents a paremeter to be used when displaying the world (some sort of scale).

Maybe a TileType ought to know its scale, and/or the Renderer should handle it (and include zooming levels etc!). In any case, it needs to leave TileMap object.

Here's an other: you're loading files from String filepath. It's a good habit to load from a File or even an InputStream, because it's not the responsibility of the file parser to know how to open or how to stream the file's content, and handle the associated exceptions. Preferably, though, the game object doesn't even know how to read or parse, it only knows its internal logic. It should be the GameSaver's job to read/write, and invoke business methods on game objects.

---

Tile Set handling

A Tile Set will have very real complexity. It must hold images obviously, also provide variety in tiles to reduce repeating patterns, maybe even have LOD. To manage it , use a proper TileSet object.

This way, you could have:

• A TileType class to handle game mechanics
• A TileSet class to handle Tile display (switchable, a la OpenTTD)
• A Tile class to handle Tile placement and mutable state (isBurning() etc.)

---

All in one shot

Here I'll present a more usable class hierarchy, but not the method's content, left as an exercise ;)

TileType.java:

public enum TileType {
 GRASS, LAND, SEA, MOUNTAIN
 public boolean isWalkable();
 public int getDefenseBonus();
}

TileSet.java

public class TileSet extends Map<TileType , Image> {
 // Could extend Map<TileType, List<Image>> to have variety
 // Could extend Map<TileType, Map<Integer, Image>> to have different resolutions of each tile
 // Could extend Map<TileType, Map<Integer, List<Image>>> to have variety at different resolutions
 public Image getImage(TileType tileType);
 public void addTileImage(TileType tile, Image image);
}

Tile.java

public class Tile {
 public int getX();
 public int getY();
 public TileType getType();
}

GameWorld.java:

public class GameWorld {
 public Tile getTile(int x, int y);
 public Tile[][] getRectArea(int x, int y, int width, int height);
 public void setTile(int x, int y, Tile tile);
}

GameSaver.java:

public class GameSaver{
 public static TileSet loadTileSetFromFile(String file);
 public static GameWorld loadWorldFromFile(String file);
 public static boolean saveToFile(TileSet tileSet);
 public static boolean saveToFile(GameWorld gameWorld);
}

• Roovy: Thank you very much! This was very helpful. I'm currently working on a version that includes all changes you and Timothy suggested. Would you mind taking a look at it when it's done (especially at the Javadocs since I'm fairly new to them...)
• Roovy: Also, somebody told me I should post the changed classes since it would be obvious to everybody seeing this to make the changes itself. But now that this got quite big, can I post the improved map? (Leaving the old map as it is) -> Just wanted to ask you, since you seemed to have a lot of experience.
• MrBrushy: @Roody Sure. When the changes are large, it help a lot. There are plenty of follow-up questions posted. You just open a new Q, present your reworked code, link back to this Q, and people can see the change, discuss it, and maybe improve further. All in all, I think a lot of people would like to see your progress.

Answer 3

I solved the problem that the map gets rendered as if it's turned 90°. That because of this line in the "drawMap" method:

g.drawImage( tileMap[row][col].getImage(), x + col * tilesize, y + row * tilesize, null );

You have to change it like this to make it render correctly:

g.drawImage( tileMap[row][col].getImage(), x + row * tilesize, y + col * tilesize, null );