2
\$\begingroup\$

The code for the LevelParser is shown below. It is also documented using Java-doc; the docs are in the project directory.

Now by improving this piece of code, I do not mean optimizing it to incomprehension, or improving its performance. What I want is to make it a bit more readable and easier to maintain. Right now, in my opinion, it is a mess.

When I was writing it, I focused only on it working and didn't care about its readability and design. And I regret it now. But I can't completely rewrite it from scratch because it is closely connected with the entire project, and that would ultimately mean rewriting the entire project.

Is there any way to only improve parts of the code, making it more readable, without a complete rewrite? Because now that it is written and bug-free, unless I find any new bugs, I won't be looking at it for a few days, I'll be focusing on other features. When I give a second look to it, say, two weeks later, I don't want to end up like this:

https://fbcdn-sphotos-a.akamaihd.net/hphotos-ak-ash4/399886_298637500185135_241806149201604_816235_1320960193_n.jpg

Code (also on GitHub):

package pro;

import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Image;
import java.awt.Point;
import java.awt.Toolkit;
import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import javax.swing.JPanel;

public class LevelParser {
    List<Sprite> tileArray = new ArrayList<Sprite>();
    Map<Character, String> tileMapping = new HashMap<Character, String>();
    Map<Character, String> messageMapping = new HashMap<Character, String>();

    Point offset = new Point(0,0);
    Dimension levelSize = new Dimension(0,0);

    Dimension tileSize;
    JPanel frame;

    /* Constructors */

    public LevelParser(Dimension tileSize, JPanel frame) {
        super();
        this.tileSize = tileSize;
        this.frame = frame;
    }

    public LevelParser( int w, int h, JPanel applet ){

        this( new Dimension(w,h), applet );
    }

    /* Other Methods */

    public void reset(){
        tileArray.clear();
        tileMapping.clear();
        messageMapping.clear();

        offset = new Point(0,0);
        levelSize = new Dimension(0,0);
    }

    /* Getters and Setters */

    /* Parser Lists */

    public List<Sprite> getTiles() {
        return tileArray;
    }

    public List<Sprite> getTileArray() {
        return tileArray;
    }

    public void setTileArray(List<Sprite> tileArray) {
        this.tileArray = tileArray;
    }

    public Map<Character, String> getTileMapping() {
        return tileMapping;
    }

    public void setTileMapping(Map<Character, String> tileMapping) {
        this.tileMapping = tileMapping;
    }

    public void setTiles(List<Sprite> tiles) {
        this.tileArray = tiles;
    }

    public Dimension getTileSize() {
        return tileSize;
    }

    public void setTileSize(Dimension tileSize) {
        this.tileSize = tileSize;
    }

    /* Members */

    public JPanel getApplet() {
        return frame;
    }

    public void setApplet(JPanel applet) {
        this.frame = applet;
    }

    public int getTileWidth(){
        return getTileSize().width;
    }

    public void setTileWidth(int w){
        this.tileSize.width = w;
    }

    public int getTileHeight(){
        return getTileSize().height;
    }

    public void setTileHeight(int h){
        this.tileSize.height = h;
    }

    public Point getOffset() {
        return offset;
    }

    public void setOffset(Point offset) {
        this.offset = offset;
    }

    /* Level Size */

    public Dimension getLevelSize() {
        return levelSize;
    }

    public void setLevelSize(Dimension levelSize) {
        this.levelSize = levelSize;
    }

    public void setLevelWidth(int w){
        levelSize.width = w;
    }

    public int getLevelWidth(){
        return levelSize.width;
    }

    public void setLevelHeight(int h){
        levelSize.height = h;
    }

    public int getLevelHeight(){
        return levelSize.height;
    }

    /* Collision Detection */

    public boolean checkCollision( AstroSprite plyr ){
        for( Sprite s : tileArray ){
            if ( s.isCollideable() && s.isCollidingRect(plyr) ){

                // if player is moving or the tiles are scrolling...
                if(plyr.getVectorY() < 0 || offset.y > 0) plyr.setTop(s.getBottom());
                else if(plyr.getVectorY() > 0 || offset.y < 0) plyr.setBottom(s.getTop());
                else if(plyr.getVectorX() < 0 || offset.x > 0) plyr.setLeft(s.getRight());
                else if(plyr.getVectorX() > 0 || offset.x < 0) plyr.setRight(s.getLeft());
            }
        }

        return false;
    }

    public Sprite getCollidingTile( Sprite plyr ){
        for( Sprite s : tileArray ){
            if ( s.isCollideable() && s.isCollidingRect(plyr) ){
                return s;
            }
        }

        return null;
    }

    public void showTileMessage(AstroSprite plyr){
        int dx = 0, dy = 0;
        if ( plyr.getDirection() == AstroSprite.LEFT ){
            dx = -2;
        }
        else if ( plyr.getDirection() == AstroSprite.RIGHT ){
            dx = 2;
        }
        else if ( plyr.getDirection() == AstroSprite.UP ){
            dy = -2;
        }
        else if ( plyr.getDirection() == AstroSprite.DOWN ){
            dy = 2;
        }

        plyr.move(dx, dy);

        Sprite s = getCollidingTile(plyr);
        if ( s != null ){
            Character c = getTileMappingKeyFromSprite( s );

            if (messageMapping.containsKey(c)){
                TextMessage.Display( messageMapping.get(c) );
            }
        }

        plyr.move(-dx, -dy);
    }

    /* Lists etc. */

    public Character getTileMappingKeyFromSprite( Sprite s ){
        /* It definitely has an Image Url, because all tile sprites'
         * Image URLs were set in the parseLine method, Note that this
         * won't work on ordinary sprites.
         */

        if ( tileMapping.containsValue( s.getImageUrl() ) ){
            return Utility.getKeyByValue(tileMapping, s.getImageUrl());
        }

        return null;
    }

    /* Drawing */

    public void drawTiles( Graphics g ){
        for( Sprite s : tileArray ){

            s.move(offset.x, offset.y);

            if( onScreen( s, frame ) ){ 
                if ( ! s.draw(g) )
                g.drawImage(s.getImage(), 
                        s.getX(),
                        s.getY(),
                        getTileWidth(), getTileHeight(), getApplet());
            }
        }
    }

    public void updateTiles(){
        for( Sprite s : tileArray ){
            if ( onScreen( s, frame ) ){
                s.update(frame);
            }
        }
    }

    boolean onScreen(Sprite spr, JPanel applet){
        if ( spr.getX() < applet.getWidth() && spr.getY() < applet.getHeight() ){
            return true;
        }

        return false;
    }

    /* Parsing */

    /* Mapping */

    public void parseTileMapping( String filename ){
        try{
            BufferedReader reader = new BufferedReader( new FileReader(filename) );

            String line;

            while( (line = reader.readLine()) != null ){

                if (line.startsWith("$")){
                    String[] tmp = line.split("=");

                    String p = tmp[0].substring(1).trim();
                    tileMapping.put( p.charAt(0), tmp[1].trim() );

                    if ( tmp.length > 2 ){
                        messageMapping.put( p.charAt(0), tmp[2].trim() );
                    }
                }

            }

        } catch(FileNotFoundException e){
            System.out.println("Could not find " + filename);
            System.exit(1);
        } catch( IOException e){
            System.out.println("Could not read line!");
            System.exit(1); 
        }
    }

    /* Level */
    public void parseLevel( String filename ){

        parseTileMapping( filename );

        BufferedReader reader;
        try{
            reader = new BufferedReader( new FileReader(filename) );
            parseTiles( reader );

        } catch(FileNotFoundException e){
            System.out.println("Could not find " + filename );
            System.exit(1);
        }

    }

    public void parseTiles( BufferedReader reader ){

        String line;

        try{

            int count = 1;
            while( (line = reader.readLine()) != null){

                if( ! line.startsWith("#") && ! line.startsWith("$") ){
                    parseLine( line, count++ );
                }
            }

        }catch(IOException e){
            System.out.println("Failed to read line!");
            System.exit(1);
        }
    }

    void parseLine( String line, int count ){
        int x = 0, y = count;

        for( int i = 0; i < line.length(); i++){

            char c = line.charAt(i);
            Image img = null;

            if (tileMapping.containsKey(c)){
                img = Toolkit.getDefaultToolkit().getImage( ConfigurationLoader.IMAGE_URL
                                                            + tileMapping.get(c) );
            }

            if ( img != null ){

                Sprite s = new Sprite( new Point(x * getTileWidth(), y * getTileHeight()),
                                       new Dimension( getTileWidth(), getTileHeight() ),
                                       img);

                /* c definitely exists, otherwise img would have been null */
                s.setImageUrl( tileMapping.get(c) );  

                if ( Character.isDigit(c)) s.setCollideable(false);

                tileArray.add( s );
            }

            x++;
            setLevelWidth( Math.max( x*getTileWidth(), getLevelWidth() ) );
            setLevelHeight( Math.max( y*getTileHeight(), getLevelHeight() ) );
        }
    }
}
\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$
public class LevelParser {
    List<Sprite> tileArray = new ArrayList<Sprite>();
    Map<Character, String> tileMapping = new HashMap<Character, String>();
    Map<Character, String> messageMapping = new HashMap<Character, String>();

    Point offset = new Point(0,0);
    Dimension levelSize = new Dimension(0,0);

    Dimension tileSize;
    JPanel frame;

At this point I'm suspicious. What does a Parser need a Panel?

    /* Getters and Setters */

I'm suspicious of getters and setters. They often indicate that other classes are too tightly coupled with this one. Do you actually need all these getters and setters? I try to minimize them.

    /* Collision Detection */

    public boolean checkCollision( AstroSprite plyr ){

What? Is there a tax on vowels? I recommend avoiding abbreviations. It doesn't really save you anything and makes your code harder to read

        for( Sprite s : tileArray ){
            if ( s.isCollideable() && s.isCollidingRect(plyr) ){

Why isCollidingRect rather then isColliding? I'd also check isCollidable inside isCollidingRect and just have it return False when its not a collidable object.

                // if player is moving or the tiles are scrolling...
                if(plyr.getVectorY() < 0 || offset.y > 0) plyr.setTop(s.getBottom());
                else if(plyr.getVectorY() > 0 || offset.y < 0) plyr.setBottom(s.getTop());
                else if(plyr.getVectorX() < 0 || offset.x > 0) plyr.setLeft(s.getRight());
                else if(plyr.getVectorX() > 0 || offset.x < 0) plyr.setRight(s.getLeft());

Vector doesn't imply movement, which I think you are saying here. I'd suggest maybe getVelocity(). This also really seems be something which should actually be player's concern. Its player whose position being modified to handle the collision, so it really ought to be the player object whose concerned with that.

            }
        }

        return false;
    }

    public void showTileMessage(AstroSprite plyr){
        int dx = 0, dy = 0;
        if ( plyr.getDirection() == AstroSprite.LEFT ){
            dx = -2;
        }
        else if ( plyr.getDirection() == AstroSprite.RIGHT ){
            dx = 2;
        }
        else if ( plyr.getDirection() == AstroSprite.UP ){
            dy = -2;
        }
        else if ( plyr.getDirection() == AstroSprite.DOWN ){
            dy = 2;
        }

Dx and dy should really be a vector object of some kind. You should also have a function that gives you a vector given a direction. That way you can get the vector you want without breaking into 4 separate cases like this.

        plyr.move(dx, dy);

        Sprite s = getCollidingTile(plyr);
        if ( s != null ){
            Character c = getTileMappingKeyFromSprite( s );

            if (messageMapping.containsKey(c)){
                TextMessage.Display( messageMapping.get(c) );
            }

The class name is LevelParser. Its clear that not really what this class is doing. You appear to performing display in this class as well as collision handling. I'd suggest that you really want to have the display code in a seperate class. }

        plyr.move(-dx, -dy);
    }

    /* Lists etc. */

    public Character getTileMappingKeyFromSprite( Sprite s ){
        /* It definitely has an Image Url, because all tile sprites'
         * Image URLs were set in the parseLine method, Note that this
         * won't work on ordinary sprites.
         */

        if ( tileMapping.containsValue( s.getImageUrl() ) ){
            return Utility.getKeyByValue(tileMapping, s.getImageUrl());
        }

Having to reverse lookup through a dictionary usually hints that somethings not quite right. The question is why you want the Character?

        return null;

Do you really want to return null? I'm guessing that shouldn't happen during execution. If so, you should throw an exception or something. There's no point handing null of to some code and getting a null pointer exception from some unrelated piece of code when you can pinpoint the problem here. }

    /* Drawing */

    public void drawTiles( Graphics g ){
        for( Sprite s : tileArray ){

            s.move(offset.x, offset.y);

So, I'm kinda gathering that you move by offset once a frame or something. That's not really suggested by the name offset. Also, its generally considered better to avoid mixing drawing and logic.

            if( onScreen( s, frame ) ){ 

Does this really help you? See your drawing API almost certainly checks the bounds for you, and doesn't draw images that won't end up being displayed. So you may not be getting any benefit out of doing this check.

                if ( ! s.draw(g) )
                g.drawImage(s.getImage(), 
                        s.getX(),
                        s.getY(),
                        getTileWidth(), getTileHeight(), getApplet());

The draw drawing really should be in the Sprite class. The sprite class is the one with all the information about how to draw.

            }
        }
    }

    public void updateTiles(){
        for( Sprite s : tileArray ){
            if ( onScreen( s, frame ) ){
                s.update(frame);
            }

I find it slightly odd that you don't update your offscreen sprites.

        }
    }

    boolean onScreen(Sprite spr, JPanel applet){
        if ( spr.getX() < applet.getWidth() && spr.getY() < applet.getHeight() ){
            return true;
        }

        return false;
    }

You don't need ifs to return bools

return spr.getX() < applet.getWidth() && spr.getY() < applet.getHeight();

Less messy.

    /* Parsing */

    /* Mapping */

    public void parseTileMapping( String filename ){
        try{
            BufferedReader reader = new BufferedReader( new FileReader(filename) );

            String line;

            while( (line = reader.readLine()) != null ){

                if (line.startsWith("$")){
                    String[] tmp = line.split("=");

tmp is a bad variable name, it tells me nothing except that its a variable.

                    String p = tmp[0].substring(1).trim();

A comment explaining why you are doing this would be good.

                    tileMapping.put( p.charAt(0), tmp[1].trim() );

                    if ( tmp.length > 2 ){
                        messageMapping.put( p.charAt(0), tmp[2].trim() );
                    }

Your parsing doesn't really check the text very completely. It'll accept a lot of variations. That might be good enough, I'd just be more strict.

                }

            }

        } catch(FileNotFoundException e){
            System.out.println("Could not find " + filename);
            System.exit(1);
        } catch( IOException e){
            System.out.println("Could not read line!");
            System.exit(1); 
        }

I'd recommend not just killing the program. Instead, I'd raise an exception indicating the failure to load the file. Sure that may just kill the program, but it feels cleaner to me. I'd also make sure to extract information from the exception object as to what went wrong. The error code might be useful.

    }

    /* Level */
    public void parseLevel( String filename ){

        parseTileMapping( filename );

        BufferedReader reader;
        try{
            reader = new BufferedReader( new FileReader(filename) );

It's a little odd to read open the file twice.

            parseTiles( reader );

parseTileMapping takes a filename and parseTiles takes a Reader. I'd suggest more consistency.

        } catch(FileNotFoundException e){
            System.out.println("Could not find " + filename );
            System.exit(1);
        }

    }

    public void parseTiles( BufferedReader reader ){

        String line;

        try{

            int count = 1;

I'd call this row so that its easier to grasp why you are counting.

            while( (line = reader.readLine()) != null){

                if( ! line.startsWith("#") && ! line.startsWith("$") ){
                    parseLine( line, count++ );
                }
            }

        }catch(IOException e){
            System.out.println("Failed to read line!");
            System.exit(1);
        }
    }

    void parseLine( String line, int count ){
        int x = 0, y = count;

Why not name the parameter y?

        for( int i = 0; i < line.length(); i++){

            char c = line.charAt(i);
            Image img = null;

            if (tileMapping.containsKey(c)){
                img = Toolkit.getDefaultToolkit().getImage( ConfigurationLoader.IMAGE_URL
                                                            + tileMapping.get(c) );
            }

Do you really want to handle the situation that the map contain a character that wasn't defined? Perhaps you should be raising an exception here.

            if ( img != null ){

                Sprite s = new Sprite( new Point(x * getTileWidth(), y * getTileHeight()),
                                       new Dimension( getTileWidth(), getTileHeight() ),
                                       img);

                /* c definitely exists, otherwise img would have been null */
                s.setImageUrl( tileMapping.get(c) );  

                if ( Character.isDigit(c)) s.setCollideable(false);

                tileArray.add( s );
            }

            x++;
            setLevelWidth( Math.max( x*getTileWidth(), getLevelWidth() ) );
            setLevelHeight( Math.max( y*getTileHeight(), getLevelHeight() ) );
        }
    }
}

Overall the class is doing too many different things. It purports to be a parser but only spends a relatively small amount of time doing that. Here is a list of your functions:

public void reset(){
public boolean checkCollision( AstroSprite plyr ){
public Sprite getCollidingTile( Sprite plyr ){
public void showTileMessage(AstroSprite plyr){
public Character getTileMappingKeyFromSprite( Sprite s ){
public void drawTiles( Graphics g ){
public void updateTiles(){
boolean onScreen(Sprite spr, JPanel applet){
public void parseTileMapping( String filename ){
public void parseLevel( String filename ){
public void parseTiles( BufferedReader reader ){
void parseLine( String line, int count ){

Firstly, I'll note that much of the code is concerned not with parsing but managing a group of sprites. So let's create a SpriteGroup class that will handle all that logic.

class SpriteGroup
    public boolean checkCollision( AstroSprite plyr ){
    public Sprite getCollidingTile( Sprite plyr ){
    public void showTileMessage(AstroSprite plyr){
    public void drawTiles( Graphics g ){
    public void updateTiles(){
    boolean onScreen(Sprite spr, JPanel applet){
    public void addSprite(Sprite sprite);

class LevelParser
    public Character getTileMappingKeyFromSprite( Sprite s ){
    public void parseTileMapping( String filename ){
    public void parseLevel( String filename, SpriteGroup sprites ){
    public void parseTiles( BufferedReader reader ){
    void parseLine( String line, int count ){

Next I'll observe that the TileMapping works somewhat seperately from the rest of the tile parsing. So I'd extract that into its own class

class TileMapping
    public Character getTileMappingKeyFromSprite( Sprite s ){
    public void parseTileMapping( String filename ){
    public Sprite spriteForCharacter(Character character);

class LevelParser
    public void parseLevel( String filename, SpriteGroup sprites ){
    public void parseTiles( BufferedReader reader ){
    void parseLine( String line, int count ){
\$\endgroup\$
3
  • \$\begingroup\$ +1, Thanks, from your code, I gather that the main problem ( along with many smaller ones) was that I was trying to do too much in a single class. So if I break the code up into the smaller classes you have suggested, along with all the other minor improvements you've highlighted, it will be better to maintain, right? \$\endgroup\$ Feb 9, 2012 at 6:09
  • \$\begingroup\$ @IntermediateHacker, that's the idea. \$\endgroup\$ Feb 9, 2012 at 15:10
  • \$\begingroup\$ @IntermediateHacker, I should note: without only a partial idea of what's going in your code my suggestions may or may not apply. Use your own judgement. \$\endgroup\$ Feb 9, 2012 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.