Hot answers tagged

39

I would include a comment at the top of the file indicating the version of the relevant software you've used. A quick comment stating "Tested with Python 3.6 (installed through Anaconda)" or something to that effect is nice to make sure everyone is on the same page. Since this is intended as teaching example, I will focus on minimizing the current ...


35

My comments are with respect to design of the game. I see responsibilities of entities are mixed up in many places Player should not initialize pieces. We can move the responsibility to board. Board and pieces doesn't need to know about players. Pieces should not handle move. Pieces can provide list of possible moves to reach the destination path but board ...


30

Without offering a deep code review (as I don't have a lot of specific Java knowledge), let's look at what a full "move" entails in chess: Player chooses piece to move. Piece makes legal move according to its own move rules. In addition to purely move-based rules, there's also capture logic, so a bishop cannot move from a1-h8 if there's a piece sitting on ...


28

About using typedefs First, don't create aliases for standard types. Just write std::string instead of str. For someone reading your code, or perhaps you yourself reading your own code half a year later, whenever one reads str one wonders "is this a std::string or some other kind of string?" Furthermore, it is not good practice to introduce very ...


20

Some quick shots a if condition written like if (booleanVariable==true) can be simplified to if (booleanVariable) you shouldn't have public variables like public boolean white; no constructor of Game, Board, Player and Piece should call super() because they are obviously not inheriting / extending any class. Some design quickshots a chessgame ...


19

There are a few traps you're getting yourself into a bit. By definition your white rook starts at a specific spot. What are you going to do for the other rook? Also you're using class inheritance White_Knight(chess_board) in a funny way. Please read this quick chapter. In short, your implying that your white knight IS a chess board. But your white ...


18

Since @spyr03's extensive (and awesome) answer did not include it, here are some small comments. You want this to be an example for your students of how code should look like. You should include a docstring with every class, method and function to detail what it does and what it's arguments and return value are. Although your code is rather self-documenting, ...


14

I spent some time last night reviewing this code. Here are some specific suggestions I can make: Move your game loop into an if __name__= "__main__": statement at the bottom of the code. Currently, if the module is imported, the person who imports it will immediately start playing chess, which isn't desirable (they may just want to know about the piece ...


14

This is a lot of work, and I don't have a lot of time, but I thought I'd throw in my two cents. So, here's what I've got for you: The Good Your models are nicely formed. More than a data store, they actually do stuff. This is good practise. You've compartmentalised the code into objects that are easy to read and follow. Good job. The Bad You're ...


14

This is not C++ Calling this C++ is disingenuous (claiming C++11 nope). This is a C program that just happens to use a few C++ objects (std::cout and std::cin) and the type std::string. This style is commonly derisively referred to as C with Classes. My problem here is that C++ is a style and the way you use the language is what differentiates it from C. ...


14

When I think back to my student days, the most crucial point for me to understand code was always the entry point. In my experience, it takes a lot of experience to understand a code concept as a whole. The untrained is used to step by step thinking and evaluation through step by step progression. I would not have understood that code because it describes ...


12

numpy In this case there is no need to use. For an 8 by 8 board, filled with strings, there is no advantage to using it, apart from the possibility to index row and column at the same time enums You have a few properties wich would be best presented as an enum. The color and the type of the piece import enum class Color(enum.Enum): WHITE = 0 BLACK = ...


11

You should separate the modelling of the game, board and pieces from the display components. (google: model-view-controller, separation of concerns). You can create a Game class that models the game and the game-logic You can create a Board class that models the internal state of the chess-board. You can create a Piece class and subclasses for each Piece, e....


11

I think it definitely makes sense to use an object hierarchy here. What's the difference between a black and a white pawn? Its colour! (which will affect which way it can move relative to the board). All pieces must have exactly one colour, and we have two to chose from: public enum PieceColour { White = 0, Black = 1 } As we've already reasoned, ...


11

Instead of using a boolean to flag if something is something (and thus implying that it isn't the other thing), consider an enum; in this case that doesn't buy you anything in terms of flexibility (since it's not as if there will ever be, say, red or purple or whatnot) but it will make your code much more clear. On the game, instead of setting black and ...


11

First observation: you should probably be thinking about separating the model of the game from the presentation of the game. For example: public interface Piece { public List<Position> posMoves(ChessBoard board, Position pos); public Icon image(); posMoves is about the game in the abstract - that's clearly a representation of the rules of ...


11

This code is improved and I see that you've added the command shoot. I think there are still some things you could do to further improve the code. Enforce the rules It's a bit disappointing to be able to win in two moves: shoot 9 16 8 1 shoot 9 16 9 1 This shouldn't be possible because the dragon should not be able to shoot across the entire board. The ...


11

Initial impression Your code is already quite good, using idiomatic APL in short clear lines that each do a single job well. Your variable names are such that you don't really need comments other than the fine description you already have at the top. Describe your result You might want to add a third comment describing the result structure: ⍝ Returns a ...


10

The thing that looked the slowest was int mask = 0; for (int i = 0; i < depth; i++) { int queen = candidates[queens[i]]; mask |= queen; // previous queens cover this column.... mask |= queen >>> (depth - i); // previous queens cover this // diagonal to the right. mask |= queen << (depth - ...


10

Lets start by naming: It should be clear that the frame doesn't have the name of the project, so instead of the class ChessProject you can call this class ChessFrame instead. It's all about separation of concerns, you should have seperate class that does the Chess logic and one that does the UI and use composition to use its features class ChessBoard{ .....


10

Does it smell? Regarding the general design: You are mixing the view and the model, which makes your code inflexible, and makes it harder to find the relevant pieces of code. the loading of the image icon seems to always be the same, I would create an abstract piece class and move it there. same goes for the getters. A boolean for the color is not optimal, ...


10

I feel @spyr03's answer is great (+1 it), but I would like to extend it a but further in a number of ways. Algorithm The suggestion that you can validate the move by checking one axis moves 2, and the other moves 1, is an interesting, but not ambitious enough solution. A knight moves in a right-angled pattern, with 2 steps on one side, and 1 on the other. ...


10

return type char should only be user for characters, as it's name says. unsigned char for characters or for aliasing other types. For everything else, use anything else. If you want (u)int8_t, use <stdint.h>, and if you don't have that header, feel free to write your own typedef (enclosed in an #if). But for error codes, the best is to use good ...


9

First, you can make your empty board much faster. For a start, you can create a list of 8 '.'s with the multiplication operator. board = ['.'] * 8 And you can make it have the 2 axes with a thing called a list comprehension. It's basically a shorthand for generating a list with a for loop.: board = [['.']*8 for _ in range(8)] Using _ in this loop is just ...


9

Switch your code up We can make this much more readable with a switch statement. switch(piece_abs_val) { case 1: if(piece_val != -1){ //w_pawn if(piece_direction == 4 || piece_direction == 6){ is_attacked = true; } } else { //b_pawn if(piece_direction == 2 || piece_direction == 8){ ...


9

using System.Text; using System.Threading.Tasks; What are these doing there? (The obvious answer would be that you used the default template in VS and didn't bother to clean up, but I hope that you have a better explanation than that). static void Main(string[] args) { ChessGame game = new ChessGame(); } When I compile ...


9

In my opinion, the best way to improve this program is by using bitboards. Instead of using a table in two dimensions to represent the chess board, you use 12 numbers of 64 bits, each number representing a type of piece and each bit saying whether there's a piece or not on a square. You can then use bitwise operators to modify the chessboard. This method is ...


8

One thought ... Instead of having all those else blocks that only have a return statement, switch your if statements around, so that the if block only has a return statement, and then there doesn't need to be an else block for it. E.g., instead of ... if 1<=X1<=8: original_contents_of_if_block else: return -1 write ... if not(1<=X1<=8)...


8

What an interesting challenge. Learning Java8 is on my list, so here's some suggestions I have, but bear in mind that I am learning too.... First up, lets use functions for the functions we have. Starting with the safe function: private static boolean safe(Pos p, Pos q) { return p.x != q.x && p.y != q.y && abs(p.x - q.x) != abs(p.y - q....


Only top voted, non community-wiki answers of a minimum length are eligible