Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 12:40

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

answered Jan 17, 2017 at 11:12

MrBrushy

• 1.3k
• 7
• 17

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);
}