5
\$\begingroup\$

The code is part of a very basic 2D platformer based on code from the book Killer Game Programming in Java.

The methods are part of a TileMapManager class which is handling basic collision detection.

  • The first method returns the greatest distance that an entity can travel in the y axis (up to deltaY) without colliding with a tile:

    public int getMaxDeltaY(Point initialPoint, int deltaY){
    
        //validate params
        if(deltaY>=tileMap.getTileHeight()) throw new IllegalArgumentException("Cannot validate if deltaY is larger than brickHeight");
        if(deltaY==0) return deltaY;
    
        int x = initialPoint.x;
        int testY = initialPoint.y + deltaY;
    
        if(isInsideTile(x, testY)){
    
            //get which row we are colliding with
            int mapRow = testY/tileMap.getTileHeight();
            //the distance between our position and
            //the top of the tile we are colliding with
            int topOffset = testY - (mapRow * tileMap.getTileHeight());
            //get the amount we should move to place
            //ourselves just next to the tile (top or bottom)
            int maxDeltaY = 0;
    
            //moving up case
            if(deltaY <0){
                maxDeltaY = deltaY + (tileMap.getTileHeight()-topOffset);
            }
            //moving down case
            else if(deltaY >0){
                maxDeltaY = deltaY - topOffset;
            }
            return maxDeltaY;
        }
        //we won't collide so it's okay to move deltaY
        return deltaY;
    }
    
  • The second method does the same thing in the x axis:

    public int getMaxDeltaX(Point initialPoint, int deltaX){
        //validate params
        if(deltaX>=tileMap.getTileWidth()) throw new IllegalArgumentException("Cannot validate if moveStep is larger than brickWidth");
        if(deltaX==0) return deltaX;
    
        int testX = (initialPoint.x + deltaX);
        int y = initialPoint.y;
    
        //if we collide
        if(isInsideTile(testX, y)){
    
            //get which column we are colliding with
            int mapCol = testX/tileMap.getTileWidth();
            //the distance between our position and
            //the left side of the tile we are colliding with
            int leftOffset = testX - (mapCol * tileMap.getTileWidth());
            //get the amount we should move to place
            //ourselves just next to the tile (left or right)
            int maxDeltaX = 0;
    
            //moving left
            if(deltaX < 0){
                maxDeltaX = deltaX + (tileMap.getTileWidth() - leftOffset);
            }
            //moving right
            else if(deltaX > 0){
                maxDeltaX = deltaX - leftOffset;
            }
    
            return maxDeltaX;
        }
        //we won't collide so it's okay to move deltaX
        return deltaX;
    }
    

The algorithm (from the book) is essentially the same so I wanted to remove the duplication.

The code differs in only a few places:

  1. use getTileWidth() or getTileHeight()
  2. use x or y to create test point
  3. use x or y to create offset
  4. use mapRow or mapCol

So a basic way could be:

  1. pass tile width/height as argument tileDimension
  2. create test point before calling method (turn initialPoint into testPoint)
  3. pass test x/y as argument testCoordinate
  4. pass row/col as argument? Can't think of good name (mapIndex?)

But now I have loads of confusing arguments and duplicate information (testCoordinate is contained in testPoint). I read that this is also a big no no, and even a beginner like me can see that this would be ugly.

Another way might be to pass the basic information and a flag for the axis, x or y. But this would still involve a bunch of arguments and then confusing switching in the body.

So I'm hoping there's a simple pattern that is used in these situations and will magically get rid of all duplication and leave a short method that's easy to read and use...

What is the best way to refactor these methods?

\$\endgroup\$
  • \$\begingroup\$ What is the type of tileMap? \$\endgroup\$ – Bruno Costa May 1 '14 at 17:32
3
\$\begingroup\$

It seems this is a job for a Parameter class.

public abstract class DeltaParameters{
  protected ? tileMap;
  protected int x, y;

  DeltaParameters(Point point, ? tileMap){
    this.x = point.x;
    this.y = point.y;
    this.tileMap = tileMap;
  }

  public int getX(){
    return x;
  }

  public int getY(){
    return y;
  }
  public abstract int getTileSideSize();
  public abstract int getMovingCoordinate();
  public abstract void offsetMovingCoordinate(int delta);
}

public class DeltaXParameters extends DeltaParameters{
  DeltaXParameters(Point point, ? tileMap){
    super(point, tileMap);
  }

  public int getTileSideSize(){
    return tileMap.getTileWidth();
  }
  public int getMovingCoordinate(){
    return x;
  }
  public void offsetMovingCoordinate(int delta){
    x += delta;
  }
}

I leave to you the job of guessing what would be DeltaYParameters :p

public int getMaxDelta(DeltaParameters dparams, int delta){
    if(delta>=dparams.getTileSideSize()) throw new IllegalArgumentException("This message is yet to be determined. It could be another method in DeltaParameters");
    if(delta==0) return delta;

    dparams.offsetMovingCoordinate(delta);
    if(isInsideTile(dparams.getX(), dparams.getY())){
        int map = dparams.getMovingCoordinate() /dparams.getTileSideSize() ;
        int offset = dparams.getMovingCoordinate() - (map * dparams.getTileSideSize() );
        int maxDelta = 0;

        if(delta < 0){
            maxDelta = delta + (dparams.getTileSideSize() - offset);
        }
        else if(delta > 0){
            maxDelta = delta - offset;
        }

        return maxDelta;
    }
    return delta;
}

Now you could call getMaxDelta just like this

getMaxDelta(new DeltaXParameters(new Point(0, 0), tileMap), 5);

What I have done here is to take the advantage of polymorphism over DeltaParameters class. Which could not be a so straigthforward method to refactor is now proven to be in a quite simple fashion imho.

\$\endgroup\$
  • \$\begingroup\$ Thanks Bruno, that's very helpful. I did think about creating a new class, but I couldn't think of the conceptual link between all the data, and that seemed to be a bad thing. Also I was wondering what the difference is between a parameter class, a data transfer object and a poltergeist (antipattern). I'm trying to follow best practices, but it can get pretty confusing! \$\endgroup\$ – mallardz May 2 '14 at 8:09
  • \$\begingroup\$ I'm going to implement this right away, but might wait to see if there are any other responses before marking it as accepted. \$\endgroup\$ – mallardz May 2 '14 at 8:10
  • \$\begingroup\$ I don't know what is a poltergeist but a DTO(data transfer object) wouldn't suit here. As you mentioned in your question you had to pass way to many parameters to refactor the methods into a single one. As per my solution you need to have logic in your classes and DTO's don't ever have any logic and much less make use of polymorphism. I called those classes parameter classes just because they encapsulate some parameter logic needed for the algorithm it's not a official name and maybe some people have other ideias of what is a parameter class. That's fine for me. Glad I could help. \$\endgroup\$ – Bruno Costa May 2 '14 at 8:22
  • \$\begingroup\$ I see, thanks again! I implemented your method and it worked very well. As I was adding the methods to the parameter class, I realised I might as well add the entire algorithm as well and then use the polymorphism to get the right variables (actually I think I used the polymorphism in the constructor to set the correct variables for the algorithm). In the end I ended up with something more of a delta manager class and snuck in some extra functionality that I was duplicating elsewhere. I added it as a new answer for what its worth. \$\endgroup\$ – mallardz May 2 '14 at 16:12
1
\$\begingroup\$

The max delta idea feels a little awkward. If the idea is to stop something from going out of bounds, there may be easier methods. But if you're following the book, it may be best to stick with it.

To me, the most natural approach to the issue would be introducing the concept of an axis. Ideally, an axis would abstract away other coordinates, translating our input for us:

interface TileMapAxis {
  boolean isInsideTile(int position);
  int getTileSize();
  int position(Point p);
}

We'll also need a way to get such an axis. Since the axis represents an axis in our tile map, that feels like the place to provide them:

class TileMap {
  // ...

  TileMapAxis getXAxis(final int y) {
    return new TileMapAxis() {
      public boolean isInsideTile(int position) {
        return isInsideTile(position, y);
      }
      public int getTileSize() {
        return getTileWidth();
      }
      public int position(Point p) {
        return p.x;
      }
    };
  }

  TileMapAxis getYAxis(final int x) {
    return new TileMapAxis() {
      public boolean isInsideTile(int position) {
        return isInsideTile(x, position);
      }
      public int getTileSize() {
        return getTileHeight();
      }
      public int position(Point p) {
        return p.y;
      }
    };
  }
}

... at which point your original functions can be hollowed out and the algorithm kept mostly in the same form elsewhere:

public int getMaxDeltaX(Point initialPoint, int deltaX) {
    return getMaxDelta(initialPoint, deltaX, tileMap.getXAxis(initialPoint.y));
}

public int getMaxDeltaY(Point initialPoint, int deltaY) {
    return getMaxDelta(initialPoint, deltaY, tileMap.getYAxis(initialPoint.x));
}

public int getMaxDelta(Point initialPoint, int delta, TileMapAxis axis) {
    final int size = axis.getTileSize();
    //validate params
    if (delta >= size) {
        throw new IllegalArgumentException("Cannot validate if delta is larger than size");
    }
    if (delta == 0) {
        return delta;
    }

    int pos = axis.position(initialPoint);
    int testPos = pos + delta;
    if (axis.isInsideTile(testPos)) {
        //get which tile we are colliding with
        int tile = testPos/size;
        //the distance between our position and
        //the start of the tile we are colliding with
        int offset = testPos - (tile * size);
        //get the amount we should move to place
        //ourselves just next to the tile (start or end)
        int maxDelta = 0;

        //moving to start case
        if (delta < 0) {
            maxDelta = delta + (size - offset);
        }
        //moving to end case
        else if (delta > 0) {
            maxDelta = delta - offset;
        }
        return maxDelta;
    }
    //we won't collide so it's okay to move delta
    return delta;
}

Depending on your style and preferences, you could also make TileMapAxis an abstract class and move the delta-limiting method there:

abstract class TileMapAxis {
    public abstract boolean isInsideTile(int position);
    public abstract int getTileSize();
    public abstract int position(Point p);

    public int getMaxDelta(Point initialPoint, int delta) {
        final int size = getTileSize();
        //validate params
        if (delta >= size) {
            throw new IllegalArgumentException("Cannot validate if delta is larger than size");
        }
        if (delta == 0) {
            return delta;
        }

        int pos = position(initialPoint);
        int testPos = pos + delta;
        if (isInsideTile(testPos)) {
            //get which tile we are colliding with
            int tile = testPos/size;
            //the distance between our position and
            //the start of the tile we are colliding with
            int offset = testPos - (tile * size);
            //get the amount we should move to place
            //ourselves just next to the tile (start or end)
            int maxDelta = 0;

            //moving to start case
            if (delta < 0) {
                maxDelta = delta + (size - offset);
            }
            //moving to end case
            else if (delta > 0) {
                maxDelta = delta - offset;
            }
            return maxDelta;
        }
        //we won't collide so it's okay to move delta
        return delta;
    }
}

As an aside: there is nothing inherently wrong with methods that have four or five parameters, provided their use is well-documented. I'd even argue that it can be cleaner from an algorithmic point of view: separating coordinating from calculating will give code that's easier to test.

Having more than two parameters is not evil, but a traffic jam of parameters can be an indication that a function is taking on too much responsibility and may need reviewing.

\$\endgroup\$
  • \$\begingroup\$ Thanks for this, I like the concept of the TileMapAxis class, although I think there might be a typo or two in the getMaxDeltaX() method. The parameter thing was an admonition in the book I'm reading, 'Clean code'. I can kind of see where they come from, as I can find using even conceptually basic methods like the drawImage() methods in Java2D confusing to write and even more to read back. On the other hand trying to force the issue seems to lead to creating worse problems... Trials & tribulations of a newb I guess. \$\endgroup\$ – mallardz May 3 '14 at 13:57
  • \$\begingroup\$ @mallardz Yusss, those were errors, good thing you caught them. :s On the parameters... it's everyone's guess and anyone's preference. As you gain experience, you'll gain more foresight into what will or will not be an issue (though it'll never be perfect). \$\endgroup\$ – JvR May 3 '14 at 14:50
0
\$\begingroup\$

This answer is based on Bruno's suggestion, but then developed from there with help from the template method pattern.

I don't know if it is the right approach but thought it might be useful to include just for reference sake.

I created a new abstract DeltaManager class:

  • this class manages all calculations and data required to find the delta to put a point alongside a brick in any axis
  • this class now contains the getMaxDelta algorithm itself
  • this method is now similar to a template method, as the basic algorithm is there, but the variables are created by calls to abstract methods that are implemented in the subclasses
  • the DeltaManagerX & DeltaManagerY implement just a few methods that return the appropriate value (ie x or y)
  • the super and subclasses are all inner classes nested in TileMapManager which allows them access to a couple of fields/methods (such as the collision detection isInsideTile(Point point))
  • this also allows me to hard codetileWidth or tileHeight into the subclasses
  • I added a little more functionality to allow the class to deal with multiple points. This is because I need to check a number of points on a sprite and find the maximum delta the sprite as a whole can move without colliding with a tile. This let me get rid of more duplicate code.

So now the process is:

  1. create a DeltaManagerX or DeltaManagerY as a DeltaManager when needed
  2. construct it with only two parameters, the requested delta you want to move & the point/points you want to move (varargs)
  3. call the getMaxDelta() method which returns the result you need

    The code is still a little funky, and the method/variable names are a bit confusing, but otherwise it seems to work quite well:

DeltaManager:

public abstract class DeltaManager{

    protected final int requestedDelta;
    protected final int tileDimension;
    protected final int[] testCoordinates;


    DeltaManager(int requestedDelta, int tileDimension, Point... initialPoints){

        //validate params
        if(requestedDelta>=tileDimension) throw new IllegalArgumentException(getDeltaLargerThanTileMessage());
        if (initialPoints==null) throw new NullPointerException("Initial points cannot be null");
        if(initialPoints.length==0) throw new IllegalArgumentException("Must pass some initial points to test");

        this.requestedDelta = requestedDelta;
        this.tileDimension=tileDimension;

        //no point initializing anything else if delta is 0
        //there can be no collisions
        if(requestedDelta == 0){
            System.out.println("WARNING: requestedDelta is 0");
            testCoordinates = null;
        }else{
            List<Point> collidingPoints = getCollidingPoints(initialPoints);
            this.testCoordinates = getRelevantCoordinates(collidingPoints);
        }
    }

    private List<Point> getCollidingPoints(Point[] initialPoints){
        List<Point> collidingPoints = new ArrayList<Point>();
        Point testPoint;
        for(Point initialPoint: initialPoints){
            testPoint = getTestPoint(initialPoint);
            if(isInsideTile(testPoint)){
                collidingPoints.add(testPoint);
            }
        }
        return collidingPoints;
    };

    private int[] getRelevantCoordinates(List<Point> collidingPoints){
        int[] coordinates = new int[collidingPoints.size()];
        int index=0;
        for(Point collidingPoint: collidingPoints){
            coordinates[index++]=getRelevantCoordinate(collidingPoint);
        }
        return coordinates;
    };  

    public int getMaxDelta(){

        if(requestedDelta==0) return 0;

        int maxPossibleDelta = requestedDelta;
        int testDelta = 0;
        for(int testCoordinate: testCoordinates){
            testDelta = getMaxDeltaFromSingleCoordinate(testCoordinate);
            if(Math.abs(testDelta) < Math.abs(maxPossibleDelta)) maxPossibleDelta = testDelta;
        }
        return maxPossibleDelta;
    }

    private int getMaxDeltaFromSingleCoordinate(int testCoordinate){

        //get which column or row we are colliding with
        int mapVector = testCoordinate/tileDimension;
        //the distance between our position and
        //the left or top side of the tile we are colliding with
        int offset = testCoordinate - (mapVector * tileDimension);
        //get the amount we should move to place
        //ourselves just next to the tile
        //(above/below or to the left/right)
        int maxDelta = 0;

        //moving left or up
        if(requestedDelta < 0){
            maxDelta = requestedDelta + (tileDimension - offset);
        }
        //moving right or down
        else if(requestedDelta > 0){
            maxDelta = requestedDelta - offset;
        }

        return maxDelta;
    }

    protected abstract Point getTestPoint(Point initialPoint);
    protected abstract int getRelevantCoordinate(Point point);
    protected abstract String getDeltaLargerThanTileMessage();

}

DeltaManagerX

public class DeltaManagerX extends DeltaManager{

    DeltaManagerX( int requestedDeltaX, Point... initialPoints){
        super(requestedDeltaX, TileMapManager.this.tileMap.getTileWidth(), initialPoints);
    }

    @Override
    protected String getDeltaLargerThanTileMessage() {
        return "Collision detection cannot work if deltaX is larger than tile width";
    }

    @Override
    protected Point getTestPoint(Point initialPoint) {
        int testX = initialPoint.x + requestedDelta;
        return new Point(testX, initialPoint.y);
    }

    @Override
    protected int getRelevantCoordinate(Point point) {
        return point.x;
    }

}

DeltaManagerY

public class DeltaManagerY extends DeltaManager{

    DeltaManagerY( int requestedDeltaY, Point... initialPoints){
        super(requestedDeltaY, TileMapManager.this.tileMap.getTileHeight(), initialPoints);
    }

    @Override
    protected String getDeltaLargerThanTileMessage() {
        return "Collision detection cannot work if deltaY is larger than tile width";
    }

    @Override
    protected Point getTestPoint(Point initialPoint) {
        int testY = initialPoint.y + requestedDelta;
        return new Point(initialPoint.x, testY);
    }

    @Override
    protected int getRelevantCoordinate(Point point) {
        return point.y;
    }
}
\$\endgroup\$
  • \$\begingroup\$ You shouldn't call methods that can be overriden in the constructor (or in their code they do so). See why here \$\endgroup\$ – Bruno Costa May 2 '14 at 16:21
  • \$\begingroup\$ Dang, every time I think I've fixed something, I seem to introduce even more problems! Thanks for the link, I have vague recollections of this when I read Effective Java (and promptly forgot it). The link suggests that it would fail immediately, but I've been running my code for a bit and the collision detection seems fine. I wonder if it is because I'm not relying on any fields or state in the overridden methods in the constructor, but they are directly passing arguments instead. I also wonder why Eclipse doesn't warn about this, seems easy enough to detect. Ok, back to the drawing board. \$\endgroup\$ – mallardz May 2 '14 at 17:25
  • \$\begingroup\$ I will be able to review your DeltaManager if you edit your question or make a follow up (which I think that could suit better here). \$\endgroup\$ – Bruno Costa May 2 '14 at 18:46
  • \$\begingroup\$ Thanks Bruno. I might post this again later on as a new question but first I have much worse code elsewhere in the game to deal with! In the meantime I've accepted your answer as the one that works and doesn't do anything potentially dangerous. \$\endgroup\$ – mallardz May 3 '14 at 14:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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