# Proper OO method for condensing code segment

This topic is 5477 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Ok, while I was writing a Board class I came across a bit of code which I have always struggled with optimizing.
/**
* Movement functions with auto
* edge detection.
*/
function Board_SouthWest( x,y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x-[1],y+[1]);
}
}
function Board_NorthWest( x, y) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x-[1],y-[1]);
}
}
function Board_North( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x,y-[1]);
}
}
function Board_South( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x,y+[1]);
}
}
function Board_East( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x+[1],y);
}
}
function Board_West( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x-[1],y);
}
}
function Board_NorthEast( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x+[1],y-[1]);
}
}
function Board_southEast( x, y ) {
var toRet = index(x,y);
if( !Board_is_Edge(x,y) ) {
toRet = Board_index(x+[1],y+[1]);
}
}


This is writen in javascript and I have many other such implementation in other languages including java, c/c++, etc... I have one implementation where I used a mathmatical parameter to one function which allow me to use a switch statement with one additional function to perform all of these movements. I am still struggling with the exact OO method for implementing this in a cleaner more optimized way. If anyone can propose a solution that would be great.

##### Share on other sites
So I've taken a stab at you're sample code. I kinda feel like i'm designing in the dark because there's no code showing how this object would be used, but I through something together based on what I thought was the intention. I'll say upfront, this isn't incredibly "object oriented" or anything. It's a simple solution that directly addresses the question at hand. I suppose you could get all crazy and overcomplicated with this, but given the paramteres of the question, I didn't see a reason to it.

I'd like some comments on my implementation. I'm curious as to what everyone feels are the pros and cons.

I didn't try compiling this yet, it should work. Regardless, the intent is clear:
class Board{public:	enum BoardDirection	{		BD_NORTH	= 1,		BD_EAST		= 2,		BD_SOUTH	= 4,		BD_WEST		= 8	};	int Index(const unsigned int x, const unsigned int y);	int IndexInDirection(const unsigned int startX, const unsigned int startY, const BoardDirection direction)	{		unsigned int destinationX = startX;		unsigned int destinationY = startY;		CalculatePosition(direction, destinationX, destinationY);		ClampValue(destinationX, 0, m_Width);		ClampValue(destinationY, 0, m_Height);		return Index(destinationX, destinationY);	}private:	void CalculatePosition(const BoardDirection direction, unsigned int& x, unsigned int& y)	{		if(BD_NORTH & direction)		{			--destinationY;		}		else if(BD_SOUTH & direction)		{			++destinationY;		}		if(BD_EAST & direction)		{			--destinationX;		}		else if(BD_WEST & direction)		{			++destinationX;		}	}	void ClampValue(unsigned int& value, int begin, int end)	{		if(value < begin)			value = begin;		else if(value >= end)			value = end - 1;	}	unsigned int m_Width;	unsigned int m_Height;};

##### Share on other sites
That works, but is over-engineered for my taste. When I have to do something like this I usually prefer to have the client code provide delta values directly; if the client code has to determine a direction, it is often easier to calculate those values directly, rather than translate to a code value (and then have to translate back again).

The original code has a problem in that Board_is_Edge doesn't know *which* edge to check against. So if you reach *any* edge of the board, suddenly you can't move *at all*. Also, OO isn't a magic bullet; the thing you should be worried about here is getting rid of all that awful duplication.

On the other hand, having a Board class does probably make some sense for whatever your application is, and provides a convenient place to store whatever information is needed to calculate the equivalent of "Board_index".

Thus:

// I hope you don't mind that I am writing Java, and according // to a rather different coding standard :)public class Board {  // I am also going to fill in a couple of the other details  // that you might expect to see in the class  private Square[][] grid;  // Java can tell us the height and width.  // Assuming the player has some location in the grid, which  // is special by virtue of the fact that the player is there  private int[] playerCoords;  // insert constructors here.  public Square squareAt(int x, int y) {    return squareInDirection(x, y, 0, 0);  }    // The exact organization of implementation vs interface  // is a language-specific issue. In Java, everything needs  // to be inline in your classes; the cultural assumption is  // that if you don't trust people, you give them your .class  // file and the javadoc. In C++ there are other considerations  // to worry about like the Pimpl idiom...  public Square squareInDirection(int x, int y, int dx, int dy) {    int[] target = clip(x+dx, y+dy);    return grid[target[0]][target[1]];    // There are various reasons why you might want to return    // a copy instead, but I find it more Java-idiomatic to    // embrace the way that objects are aliased. Of course,    // I don't really have large-project experience...  }  public Square playerSquare() {    return grid[playerCoords[0]][playerCoords[1]];  }  public void movePlayer(int dx, int dy) {    playerCoords = clip(playerCoords[0]+dx, playerCoords[1]+dy);  }  private static int[] clip(int x, int y) {    int[] result = new int {x, y};    int h = grid.length;    int w = grid[0].length;    if (result[0] < 0) result[0] = 0;    if (result[0] >= h) result[0] = h - 1;    if (result[1] < 0) result[1] = 0;    if (result[1] <= w) result[1] = w - 1;    return result;  }}

Although, there seems to be a common theme now of using int[]'s to represent coordinates. Time to make a new class to represent a coordinate:

public class Coordinate {  private int w;  private int h;  public Coordinate(int w, int h) {    this.w = w; this.h = h; // C++ got this one right. Sigh.  }  public void clipTo(Coordinate mins, Coordinate maxes) {    if (w < mins.w) w = mins.w;    if (w > maxes.w) w = maxes.w;    if (h < mins.h) h = mins.h;    if (h > maxes.h) h = maxes.h;  }  public void add(Coordinate other) {    w += other.w; h += other.h;  }  public Coordinate sum(Coordinate other) {    return new Coordinate(w + other.w, h + other.h);  }  // This is perhaps cleaner than having public data or using  // accessors... in Java 1.5, we can avoid casting via generics.  public Object access(Object[][] grid) {    return grid[w][h];  }}// And now the Board class becomes simpler:public class Board { // in a separate file  private Square[][] grid;  private Coordinate bottomRight;  private Coordinate playerLoc;  private static Coordinate origin = new Coordinate(0,0);  // Or maybe the Coordinate class will implement some Factory  // pattern instead...  public Board(int x, int y) {    grid = new Square[x][y];    // initialize further    bottomRight = new Coordinate(x-1, y-1);  }  public Square squareAt(Coordinate loc) {    return squareInDirection(loc, origin);  }    public Square squareInDirection(Coordinate loc, Coordinate direction) {    Coordinate target = loc.sum(direction);    target.clipTo(origin, bottomRight);    return (Square)(target.access(grid));  }  public Square playerSquare() {    return (Square)(playerLoc.access(grid));  }  public void movePlayer(Coordinate delta) {    playerLoc.add(delta);    playerLoc.clipTo(origin, bottomRight);  }}

[Edited by - Zahlman on September 14, 2004 10:54:01 PM]

##### Share on other sites
Quote:
 When I have to do something like this I usually prefer to have the client code provide delta values directly; if the client code has to determine a direction, it is often easier to calculate those values directly, rather than translate to a code value (and then have to translate back again).

I'm curious what your rational is behind that statement.

The way I see it is that by having the client code specify named-constants for directions it:
--1) makes the code more explicit.
--2) reduces the duplication of the client code for calculating the direction.
--3) explicitly forces a direction of 1 unit per move (may or may not be important).

In all fairness, regarding my solution, I will say that requiring the use of bitflags for the direction is not 100% intuitive. But, being that we're all programmers here, it should be. The alternative solutions are provide four bools, one for each direction, or have one function for each direction. Both of those solutions seem to be overcomplicating the situation. Bitflags, while not very "high level" are certainly a viable, simple, solution. -- I'd like to hear some thoughts about that!

***************************************************************
warning: this next point I'm going to make is incredibly nitpicky, but it is the type of issue which, imo, can make or break the ease of use of an interface.

Zahlman please don't take offense to this, I'm not intending it as a personal attack. I'd like to hear your rationale about the points I'm making. This is a place for intelligent discussions!
***************************************************************
I can see the usefulness of your solution. However, in both 'squareInDirection' and 'movePlayer' you use a Coordinate for the direction, and the amount to move. The naming of the type for those functions leads me to two conflicting conclusions about their behavior:
--1) the coordinate is the absolute destination position.
--2) the coordinate encodes the relative distance and direction.

It just seems odd to me to be using a 'Coordinate' when you really want a Vector. The difference may be subtle, but one should not have to inspect the code of a method to understand how to use it.

The choice of clipTo for the coordinate method seems a bit misleading as well. Clipping refers to discarding something if it is outside of a range. However, the method is clamping the coordinate, not clipping it. clampInBounds(), or something more explicit, would be more intention revealing.

The constructor for Coordinate takes w and h. Are those short for width or height? I would have thought x and y.

The constructor for the Board takes x and y. Are those the coordinates of the origin? If so, why not just take a Coordinate instance? Or, are those the width and height?

Looking at the code, or using some common sense, tells me the answers to the the questions I posed. Others may not be so lucky, and this is a pretty trivial example.

** My point: Have Intention Revealing Interfaces

##### Share on other sites
I'll take a crack at it:

struct Interface	{	virtual ~Interface() {}	};struct vector2D	{	vector2D(int x=0, int y=0) : x(x), y(y) {}	int x,y;	};struct IPiece : virtual Interface	{	//Absolute location on the board	virtual vector2D Location()=0;		//Check if this piece can make this type of move	virtual bool ValidMove(vector2D relative_movement)=0;		//Move the piece (no error checking, just move it)	virtual void Move(vector2D position)=0;		//Put the piece in this exact spot, useful for board reset	virtual void Place(vector2D position)=0;		//List of valid (relative) movements this piece can make	//Useful for highlighting valid moves on the board	virtual const std::vector<vector2D>& ValidMoves()=0;	};struct IBoard : virtual Interface	{	virtual void SetExtents(vector2D extents = vector2D(8,8))=0;	virtual bool ValidSquare(vector2D position)=0;	 	//Return the board location given a point of incident relative to the	// center of the board (assuming raster graphics, with 3D/vector graphics	// we'd need to use float's for the point-of-incident	virtual vector2D Pick(vector2D screen_coordinates)=0;	};struct IPlayer : virtual Interface	{	virtual std::vector<IPiece*>& Pieces()=0;	};struct IGame : virtual Interface	{	virtual bool EmptySquare(vector2D location)=0; 	virtual IPlayer* Player(int index)=0; 	virtual IBoard* Board()=0;	 	virtual IPiece* PickPiece(vector2D screen_click, int player)=0;	virtual bool TryMove(IPiece* picked_piece, vector2D picked_location)=0;	};IPiece* Game::PickPiece(vector2D screen_click, int player)	{	//Find the square clicked	vector2D picked = this->Board()->Pick(screen_click);	if(!this->Board()->ValidSquare(picked))		return 0;		//See if there's a piece (of our player) on that square	std::vector<IPiece*>& pieces = this->Player(player)->Pieces();	for(size_t i=0; i<pieces.size(); ++i)		{		if(picked == pieces->Location)			return pieces;		}	return 0;	}//From the geometry engine, a piece has been picked as well as a potential move locationbool Game::TryMove(IPiece* picked_piece, vector2D picked_location)	{	if(!this->EmptySquare(picked_location))		return false;		vector2D movement = picked_piece->Location() - picked_location;	if(!picked_piece->ValidMove(movement))  		return false;	if(this->Board()->ValidSquare(picked_location))		return false;		//Looks like a good move	picked_piece->Move(movement);	return true;	}

##### Share on other sites
Quote:
 Original post by ClashSo I've taken a stab at you're sample code. I kinda feel like i'm designing in the dark because there's no code showing how this object would be used, but I through something together based on what I thought was the intention. I'll say upfront, this isn't incredibly "object oriented" or anything. It's a simple solution that directly addresses the question at hand. I suppose you could get all crazy and overcomplicated with this, but given the paramteres of the question, I didn't see a reason to it. I'd like some comments on my implementation. I'm curious as to what everyone feels are the pros and cons.I didn't try compiling this yet, it should work. Regardless, the intent is clear:
Does it fullfill the specification?

##### Share on other sites
Quote:
 Does it fullfill the specification?

Based upon the original post, I believe so! Correct me if I misunderstood, but only the functions regarding movement, clamping to the edges, and getting the current index at a cell are used in the original post.

Also, I'd like to hear some responses to the points I made in my second post.

Thanks :D

[Edited by - Clash on September 16, 2004 12:18:10 AM]

##### Share on other sites
Oops, I didn't see 5minute gaming's post. I have my filter set too high and saw your post first and thought it was just some random code you wanted people to comment on. It seemed strange.

##### Share on other sites
First of all I would like to thank all of you for responding with very interesting implementations. Clash's implementation is a low-level solution. One that I would have tried but I was looking to get ride of that awefully huge if...else or a switch statment. I was looking for high cohesion and low coupling of the functionality.

Zahlman's implementation seems to be what I would be after if I was not using javascript but seen as how I am and I don't want a very large system I solved the problem pretty much the same way. Except for the different classes an such.

I was originally looking for a algorithm to find all the tiles on a horizantal line from the given x and y coordinates as well as all the tiles on a negative diagnol slope and a positive slope as well as a vertical slope. The reason being is that I wanted to be able to compare each index within those on the line but I didn't need to compare all the indexes of the entire board if you follow me.

I developed a recursive method which could be simplified if you wanted to do single step movements but I'll post it simply because it was my solution.

get_Tiles_In_Direction( x, y, dx, dy, lst ) { lst.add( Board_Index( x, y ) ); // add this tile // if the next tile is not outside the board then // move to the tile. if( !isOverEdge( x+dx, y+dy ) ){  get_Tiles_In_Direction( x+dx, y+dy, dx, dy, lst ); }}

There are many ways that this can be done but this function suits my purposes. For the exact OO method its completely up in the air. Thanks again for responding!

##### Share on other sites
Quote:
Original post by Clash
Quote:
 When I have to do something like this I usually prefer to have the client code provide delta values directly; if the client code has to determine a direction, it is often easier to calculate those values directly, rather than translate to a code value (and then have to translate back again).

I'm curious what your rational is behind that statement.

The way I see it is that by having the client code specify named-constants for directions it:
--1) makes the code more explicit.
--2) reduces the duplication of the client code for calculating the direction.
--3) explicitly forces a direction of 1 unit per move (may or may not be important).

In all fairness, regarding my solution, I will say that requiring the use of bitflags for the direction is not 100% intuitive. But, being that we're all programmers here, it should be. The alternative solutions are provide four bools, one for each direction, or have one function for each direction. Both of those solutions seem to be overcomplicating the situation. Bitflags, while not very "high level" are certainly a viable, simple, solution. -- I'd like to hear some thoughts about that!

The problem is that with bitflags you can do silly things like NORTH|SOUTH, and also you have these ugly constants hanging around, and you have to do this kind of translation.

I like providing deltas because (a) as I mentioned, they're often easier to get at, and avoid back-and-forth converstion and (b) if later on you need to support other methods of movement, your task is quite simplified. Since you have to create the concept of "a direction" anyway, I'd rather not have to make some weird encoding.

Of course, I don't explicitly enforce the directions of movement, but that can be done easily enough - but I have to address the next point first...

Quote:
 Zahlman please don't take offense to this, I'm not intending it as a personal attack. I'd like to hear your rationale about the points I'm making. This is a place for intelligent discussions!

No worries, I don't let things like this bother me even if they should, and here it shouldn't, because I know for a fact that I don't always do the best, OO-ideological thing - KISS is my #1 principle and I really only like object orientation to the extent that it helps me with that. Although I certainly can design extra stuff if it's warranted :)

Quote:
 I can see the usefulness of your solution. However, in both 'squareInDirection' and 'movePlayer' you use a Coordinate for the direction, and the amount to move. The naming of the type for those functions leads me to two conflicting conclusions about their behavior:--1) the coordinate is the absolute destination position.--2) the coordinate encodes the relative distance and direction.It just seems odd to me to be using a 'Coordinate' when you really want a Vector. The difference may be subtle, but one should not have to inspect the code of a method to understand how to use it.

Yes, this is the main ugly thing. It is common enough to confuse a point and a direction, and in the interest of writing up a simple example I didn't bother to make a distinction.

A Direction doesn't do nearly as much as a Coordinate, but it has the same sort of encoding and it seems a shame not to make these two classes related. Although I would certainly hear an argument for making them two completely different classes which coincidentally have x and y members; except that that makes it difficult to do the addition.

Quote:
 The choice of clipTo for the coordinate method seems a bit misleading as well. Clipping refers to discarding something if it is outside of a range. However, the method is clamping the coordinate, not clipping it. clampInBounds(), or something more explicit, would be more intention revealing.

Er. Yeah, I guess so. Somehow I don't really like clampInBounds() as a name either, though. How about limitTo()?

Quote:
 The constructor for Coordinate takes w and h. Are those short for width or height? I would have thought x and y.

They are, and I don't know why I made them w and h. :/

Quote:
 The constructor for the Board takes x and y. Are those the coordinates of the origin? If so, why not just take a Coordinate instance? Or, are those the width and height?

Yeah. *Those* should have been the w and h. Brain fart. :s

Now then, presenting: some checking for valid Directions of movement...

public class Direction {  private int x;  private int y;  protected Direction(int x, int y) {    this.x = x; this.y = y;  }  // client classes cannot make base Directions, which ensures  // validity. But we allow construction of Coordinates.  // I am not going to bother with accessors here because  // it seems overkill for final, immutable members.  public static final Direction NW = new Direction(-1, -1);  public static final Direction N  = new Direction( 0, -1);  public static final Direction NE = new Direction( 1, -1);  public static final Direction  W = new Direction(-1,  0);  public static final Direction  E = new Direction( 1,  0);  public static final Direction SW = new Direction(-1,  1);  public static final Direction S  = new Direction( 0,  1);  public static final Direction SE = new Direction( 1,  1);  // You might find it useful to define the (0,0) direction too  // We could also implement a full Flyweight pattern to avoid  // creating all of these instances until they're used. But  // for only 8-9 of them...}public class Coordinate extends Direction {  public Coordinate(int x, int y) {    // I *think* it's valid to make this public...    super(x,y);  }  public void limitTo(Coordinate mins, Coordinate maxes) {    if (x < mins.x) x = mins.x;    if (x > maxes.x) x = maxes.x;    if (y < mins.y) y = mins.y;    if (y > maxes.y) y = maxes.y;  }  // PROBLEM: We are not protected from adding other  // Coordinates - or other Direction subclasses.  // To do that cleanly would mean inverting the hierarchy -  // but then Coordinates would be addable to other Coordinates,  // and that's really bad because our Coordinate objects  // are supposed to be immutable.  // So that would motivate the "two separate classes" approach,  // but then you have to play games in order to do the actual  // addition without breaking encapsulation. AARGH.  public void add(Direction other) {    x += other.x; y += other.y;  }  public Coordinate sum(Direction other) {    return new Coordinate(x + other.x, y + other.y);  }  // This is perhaps cleaner than having public data or using  // accessors... in Java 1.5, we can avoid casting via generics.  public Object access(Object[][] grid) {    return grid[x][y];  }}public class Board {  private Square[][] grid;  private Coordinate bottomRight;  private Coordinate playerLoc;  private static Coordinate origin = new Coordinate(0,0);  // Or maybe the Coordinate class will implement some Factory  // pattern instead...  // As pointed out, we should have width/height here, and  // x/y in the actual coordinates/directions. Meh.  public Board(int w, int h) {    grid = new Square[w][h];    // initialize further    bottomRight = new Coordinate(w-1, h-1);  }  public Square squareAt(Coordinate loc) {    return squareInDirection(loc, origin);  }    public Square squareInDirection(Coordinate loc, Direction d) {    Coordinate target = loc.sum(d);    target.limitTo(origin, bottomRight);    return (Square)(target.access(grid));  }  public Square playerSquare() {    return (Square)(playerLoc.access(grid));  }  public void movePlayer(Direction d) {    playerLoc.add(d);    playerLoc.limitTo(origin, bottomRight);  }}

Better? :)

• ### Game Developer Survey

We are looking for qualified game developers to participate in a 10-minute online survey. Qualified participants will be offered a \$15 incentive for your time and insights. Click here to start!

• 15
• 22
• 17
• 46