wood burning stoves 2.0*
The moose likes Beginning Java and the fly likes More succinct way to write this code? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Android Security Essentials Live Lessons this week in the Android forum!
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "More succinct way to write this code?" Watch "More succinct way to write this code?" New topic
Author

More succinct way to write this code?

Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

I'm writing a chess game and am currently on the code for working out possible moves for the different pieces. Below is the code I wrote for the Bishop, which moves diagonally.

It's written in the Bishop (extends Piece) class, and takes a Board object and a Square object. A Board has a Square[][] array called "grid", and each Square has a field called thisPiece that may contain a Piece. The Square passed in indicates the position of the Piece. (I know it's bad style not to encapsulate these fields but I'm trying to do this quickly and not clutter my code with getters and setters.) We look at each of the directions the Piece can move in and if a Square represents a possible move, we add an item to our PossMoveList (extends ArrayList<Square>).

I think I need 4 "for" loops, 1 for each diagonal direction extending away from the piece's current position. But the code to test whether the square is occupied / contains a friendly or enemy piece is the same in each of the four cases, so as you can see, I have the same three lines copy + pasted, which makes it less easily maintainable. It will also be pretty similar for the Queen and Rook (well, the same checking lines will be repeated 8 times for the Queen!), but different for the other Pieces. Is this the best way to write it? Or should I somehow add a separate method for checking the Square's contents?

If none of the above made any sense, it doesn't matter - I'm just looking for a way to repeat a bit of code used in multiple "for" loops. In ZX Spectrum Basic, where I learnt to code, I would use a GOSUB...

Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3599
    
  14

I think it would probably be cleaner to do something like this:
Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

Thanks for that interesting idea. I haven't tried using enums like this before, although it seems appropriate here.

However I'll still need to implement the getPossMoveList method as above for each of these. I suppose a neater way might be to produce a list of lists of squares in each direction and afterwards use a common method to loop through those to remove the "blocked" ones. Or maybe for simplicity, copy and paste is the way!
Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3599
    
  14

[EDIT]

Sorry, I misunderstood your first post.

No, I don't think you can write the code in a less verbose manner. It's fine as it is, though maybe you can make it a bit more clear.
Also, I can see that your Board does not encapsulate its grid completely. Consider making it private, and adding a method like this:


Finally, don't forget to check whether the king is unguarded. Pieces can't move if they guard the king.
Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

Stephan van Hulst wrote:
Also, I can see that your Board does not encapsulate its grid completely. Consider making it private, and adding a method like this:


heh, yes, I know, but I'm lazy - just seems like extra work since I'm never going to use these classes for anything other than this project.

On this subject though, is it appropriate to use direct references to the fields in other methods within that class? Or would you always use getters / setters even within the class itself?

Finally, don't forget to check whether the king is unguarded. Pieces can't move if they guard the king.

Good point. But I don't think it matters too much since even if I don't make it technically illegal, the evaluation of the position produced would give you zero equity so that move would be discarded.

En-passent and castling however, is going to be a pain...
Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3599
    
  14

Luigi Plinge wrote:heh, yes, I know, but I'm lazy - just seems like extra work since I'm never going to use these classes for anything other than this project.

Still, if I may give you advice, I would get used to doing it. It will make it more natural for you to do it in projects where it *does* matter, and regardless of whether you won't use the classes anymore, it will be easier for you to write client classes (like AI classes) that don't introduce bugs.

On this subject though, is it appropriate to use direct references to the fields in other methods within that class? Or would you always use getters / setters even within the class itself?

Yes, as a matter of fact, I prefer this approach. It is more clear, and you avoid 'self-use', which can break your code if the class and the setters/getters are not final.
edit: By yes I mean, use fields directly.

Good point. But I don't think it matters too much since even if I don't make it technically illegal, the evaluation of the position produced would give you zero equity so that move would be discarded.

I think you would do well to make the moves illegal, because not only do AIs benefit from it directly, you also make sure that the game won't end prematurely if a human player accidentally makes a wrong move.

En-passent and castling however, is going to be a pain...

Yes, but fun to solve :P
Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

Stephan van Hulst wrote:I think it would probably be cleaner to do something like this:


I'm having a go at doing it this way, but I'm having trouble using the fields from the ChessPiece class... I was trying to use "this.colour", which isn't recognized - I guess because "this" must refer to the enum rather than the object... but just using "colour" or the getColour() method gives me the error "non-static variable colour cannot be referenced from a static context".

Any ideas? I mean, colour can't be static because it has to be assigned to each ChessPiece... but does this mean I can't use enums?
Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3599
    
  14

Ralph Cook
Ranch Hand

Joined: May 29, 2005
Posts: 479
Lots of style opinions to be had here, but I would personally prefer this one:


The original code had too much cut-and-paste in it for my taste, and one redundant test per section. You can boil down what you want to two tests: is the square clear of a piece, and if not, is the piece of opposite color? (or colour?)

Obviously the routine can then be used for rooks and queens as well.

Anyway. This is another way to do it (I think -- I haven't even tried to compile this...)

p.s. I think a class per piece is definitely in order -- you need more than enums here, you need code that is specific to each piece for all kinds of reasons.

rc
Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

Thanks Ralph... good point about one test being redundant. I'm about to go to bed but I'll have a play with your code tomorrow.

I'm not too familiar with how much you can do with enums so I'll have to read up on that - they seem a bit tricky so far so I might just stick to normal classes like you suggest.

Screenshot from test!

Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

Ralph - you will be happy to hear that your code works perfectly (just needed to add a couple of missing ")" and capitalise the "p" in "PossMoveList"). Congrats! & thanks again.

I'm now onto the Knight and King, which will require a bit of a diiferent approach I think.
Ralph Cook
Ranch Hand

Joined: May 29, 2005
Posts: 479
Luigi Plinge wrote:Ralph - you will be happy to hear that your code works perfectly (just needed to add a couple of missing ")" and capitalise the "p" in "PossMoveList"). Congrats! & thanks again.

I'm now onto the Knight and King, which will require a bit of a diiferent approach I think.


Speaking of different approaches, I'll throw this out to you just for grins. I saw it in a chessplaying program I studied many years ago which was written in assembler and ran on some of the first 8-bit micros -- processing power and memory at a premium!

In fact, as you may already know, speed is paramount in chess-playing code (at least code that does its own moves) because of the huge amount of work involved in generating move lists and evaluating positions. So this technique helps address this.

Imagine a 12x12 square; number each square sequentially starting with the upper left corner, going across, then starting with the number 13 on the second row. You can represent such a square with a single dimension array, indices indicating the square numbers.

You can think of this as a chess board with two extra rows of squares on each of the four sides. The two extra rows are illegal squares to move to; a move that lands on one of these squares is not legal, and does not involve a calculation to determine whether it is legal. My so-called "vector pieces" will eventually hit an edge, knights cannot move beyond two rows, etc.

A chess move from any square will be a simple addition of a number to the number of the square; a knight on square 78 can move to 53, 55, 64, 68, 88, 92, 101, and 103. The differences between 78 and those numbers give you the amounts to add (positive or negative numbers) to whatever square the knight is on for the knight moves. You can do similar things to move one square diagonally in one direction, or one square along a rank or file.

The advantage of this is that you are implement the whole thing with the single array, don't have to double-index, and don't have to keep checking your indices to see if you're off the board, then check the square itself to see what to do next. You check the square, if it's an illegal square, then you're done (perhaps in an OO world, those are null in an array of squares, easy and fast to compare to, and a somewhat intuitive meaning).

Anyway, I thought you might be interested; if you've got questions, I'll check back later to answer, but am on lunch at my job and can't do more chess programming right now.

rc
Luigi Plinge
Ranch Hand

Joined: Jan 06, 2011
Posts: 441

That's an interesting way of doing it. I haven't thought too much about speed, but if speed is important it might be better to pre-create a static list of possible moves for each piece type from each square on the board, then strike out any that have other pieces on them.

I could also do something similar for Bishops, Rooks and Queens although I'd need to store these as lists of lists (for each move direction) so I could iterate through them until I hit another piece.
 
wood burning stoves
 
subject: More succinct way to write this code?
 
Similar Threads
I don't know what's wrong with this chess game
MultiDimensional array doesnt work
How do i print a table with multidimensional array.
Help with a chess program