Is this a good way to implement a tile map from a text file?

Question

I'm working on a small final fantasy type game. I have this for a tile map. I was just wondering if this is a good way to do this or if anyone has any suggestions, I would love to read them.

import com.stardust.main.gfx.Assets;
import java.awt.Graphics;
import java.awt.image.BufferedImage;
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
public class TileMap
{ 
 //placeholder for each tile 
 BufferedImage[] tiles = new BufferedImage[37];
 //array of tile types
 int[][] map = new int[32][32];
 //position coordinates
 public static int posX, posY;
 //Edge of map Cooridinates
 public static int sx, sy;
 public String fileName;
 public TileMap(int posX, int posY, String fileName) throws IOException
 {
 this.fileName = fileName;
 int x = 0, y = 0;
 this.posX = posX;
 this.posY = posY;
 this.sx = map.length * 32;
 this.sy = map.length * 32;
 BufferedReader in = new BufferedReader(new FileReader(fileName));
 String line;
 while((line = in.readLine()) != null)
 {
 String[] values = line.split(",");
 for(String str : values)
 {
 int str_int = Integer.parseInt(str);
 map[x][y] = str_int;
 y += 1;
 }
 x += 1;
 y = 0;
 }
 in.close();
 }
 public void update()
 {
 }
 //Game.mapX and Game.mapY are the position variables in the main Game class
 public void render(Graphics g)
 {
 for(int x = 0; x < 32; x++)
 { 
 for(int y = 0; y < 32; y++)
 {
 int textureType = map[x][y];
 setTile();
 BufferedImage texture = tiles[textureType]; 
 g.drawImage(texture, posX, posY, null);
 posY += 32;
 }
 posX += 32;
 posY = Game.mapY;
 }
 posX = Game.mapX;
 posY = Game.mapY;
 }
 public void setTile()
 {
 tiles[0] = Assets.grass;
 tiles[1] = Assets.dirt;
 tiles[2] = Assets.water;
 tiles[3] = Assets.tree;
 tiles[4] = Assets.multiTree;
 tiles[5] = Assets.NSpath;
 tiles[6] = Assets.uprtPath;
 tiles[7] = Assets.EWpath;
 tiles[8] = Assets.upltPath;
 tiles[9] = Assets.dnltPath;
 tiles[10] = Assets.dnrtPath;
 tiles[11] = Assets.watgrsdn;
 tiles[12] = Assets.watgrslt;
 tiles[13] = Assets.watgrsup;
 tiles[14] = Assets.watgrsrt;
 tiles[15] = Assets.watgrsSE;
 tiles[16] = Assets.watgrsSW;
 tiles[17] = Assets.watgrsNW;
 tiles[18] = Assets.watgrsNE;
 tiles[19] = Assets.wallWood;
 tiles[20] = Assets.pointer;
 tiles[21] = Assets.pathOpRt;
 tiles[22] = Assets.pathOpLt;
 tiles[23] = Assets.pathOpDn;
 tiles[24] = Assets.pathOpUp;
 tiles[25] = Assets.pathOpAll;
 tiles[26] = Assets.roughWood;
 tiles[27] = Assets.pathOpen;
 tiles[28] = Assets.counter;
 tiles[29] = Assets.storeDude;
 tiles[30] = Assets.pot;
 tiles[31] = Assets.singleHouse;
 tiles[32] = Assets.doubleHouse;
 tiles[33] = Assets.wepBanner;
 tiles[34] = Assets.armBanner;
 tiles[35] = Assets.innBanner;
 tiles[36] = Assets.blankTile;
 }
}

• o11c: Not directly related to your code, but: consider how you are going to create the maps. It's a bad idea to simplify your parser for a format that no one uses, when you could write a bit more code and deal with a standard format that already has advanced tooling. I've contributed to one: Tiled Map Editor, and it already has Java APIs so the integration cost is probably not much.
• Twiggy: Well, I was following a tile map tutorial and this was the format that was shown, I had no idea there was a standard format for tile maps. Beside that, I hadn't planned on others using my code, it's just me alone making a game.
• o11c: Your first thought should always be "has someone already solved this problem?" Laziness is one of the Three Virtues. And even it no one else uses it, it is so much easier if you don't have to invent your own map editor or worse, lay them all out by hand.
• Twiggy: Just because someone has already solved a problem doesn't mean I don't want the experience of solving that problem myself. It was something I wanted to try. If you ask me that's almost the opposite of laziness.
• 200_success: Follow-up question

Answer 1

Single responsibility principle

This class is responsible for one too many things:

• Model a tile map
• Parse a text file

As such it violates the single responsibility principle. I suggest to move the file parsing code somewhere else. For example in a TileMapFactory.fromFile method, or TileMapFileParser.

In general, you know something's not right when the constructor is too big. If it has to do IO, which is especially troublesome because of exception handling and resource management, that's really way too much burden for a constructor.

Prefer List instead of arrays

If you don't need specifically an array, then use a List instead:

List<BufferedImage> tiles = new ArrayList<>();

It will make your life easier. For example, when adding the tiles, you won't need to worry about correctly incrementing the array indexes:

 tiles.add(Assets.grass);
 tiles.add(Assets.dirt);
 // ... and so on

Magic numbers

The number 32 keeps coming up at many places in the code. Declare it as a static final variable with a meaningful name.

Get rid of unused stuff

For example this method is pointless, remove it:

 public void update()
 {
 }

Coding style

Instead of y += 1, use ++y.

• Twiggy: Thank you for all of the suggestions, I will be using a final variable for 32 and moving the file parser to it's own method. I do have a question about the list though. I'm guessing from reading a bit about it, that it will essentially be the same as it is a the moment for iteration just setting it will be different?

Answer 2

1. You should hide your fields as much as you can, declare your fields private

   private BufferedImage[] tiles = new BufferedImage[37];
   private int[][] map = new int[32][32];
   

2. public static variables that are not final are equivalent to global variables, and global variables are evil.

   private int posX, posY;
   private int sx, sy;
   

3. You should free your resources properly, in java 7:

    try(BufferedReader reader = new BufferedReader(new FileReader(fileName))){
    ...
    }
   

4. use constants instead of numbers

   private static final MAP_SIZE = 32;
   int[][] map = new int[MAP_SIZE][MAP_SIZE];
   

• Twiggy: As janos suggested I will be changing the 32 to a constant. but the public posX, posY and sx and sy are public only because I was using them in other places, but I forgot to change them to private after I changed it.
• Boris the Spider: Files.newBufferedReader is preferred to calling all the constructors directly.

Answer 3

I don't see any reason for why you want to call setTile(); in a loop. Calling it once is enough. And there's definitely no reason to call it inside the render method. Call it in the constructor instead.

Additionally, it is possible to initialize that array in a more compact way.

tiles = new BufferedImage[]{ Assets.grass, Assets.dirt, Assets.water, Assets.tree,
 Assets.multiTree, Assets.NSpath, ..., Assets.innBanner, Assets.blankTile };

Or, if you want to use a List:

tiles = Arrays.asList(Assets.grass, Assets.dirt, Assets.water, Assets.tree,
 Assets.multiTree, Assets.NSpath, ..., Assets.innBanner, Assets.blankTile);

Instead of having the setTile method return void you can have it return an array or a list, such as:

return new BufferedImage[]{ Assets.grass, Assets.dirt, Assets.water, Assets.tree,
 Assets.multiTree, Assets.NSpath, ..., Assets.innBanner, Assets.blankTile };

Then you can do:

tiles = setTile();

But I would recommend changing the name of that method to initializeAssetArray();

• Twiggy: You're correct. Thank you. For the moment I only moved it to the constructor, but later I may change it to return an array or a list. Thank you