Шахматный движок в C ++

Я ищу опытную честную обратную связь по моему движку в шахматы. Я не знаю, что бы вы искали, но моя цель - стать лучшим программистом.

board.h

/*
    File: board.h

    Content on and manipulation of a chess board implemented as 10x12 squares,
    with default out_of_border values.

    "magic" numbers will be used, they aren't magic if you know this structure.
    Source way too messy with a lot of variable names and variable calculations instead of plain numbers.

    Author: Ken Rouwas
    Date 2017-09-14
*/

#pragma once

#include <array>

namespace Chess{

    const size_t board_size = 10*12;

    enum class Color { white, black, none };

    enum class Piece {
        king,       // a king without castle potential
        king_castle,    // a king with castle potential
        queen,
        pawn,       // a pawn without en passant potential
        pawn_en_passant, // a pawn with en passant potential
        rook,
        rook_castle,
        knight,
        bishop,
        none,
        out_of_board    // Illegal position
    };

    struct Square {
        Color piece_color;
        Piece piece;
        Square(Piece, Color);
        Square();
    };

    class Board {
        private:
            std::array<Square, board_size> squares;
        public:
            void set(const size_t where, Square);
            Square get(const size_t where) const;
    };

    void init_classic_board(Board&);

}

Board.cpp

// File: board.cpp

#include "board.h"


namespace Chess {
    Square::Square(Piece p, Color c){
        piece = p;
        piece_color = c;
    }

    Square::Square(){
        piece = Piece::out_of_board;
        piece_color = Color::none;
    }


    void Board::set(const size_t where, Square s) {
        if (where >= board_size)
            return;
        squares[where] = s;
    }

    Square Board::get(const size_t where) const {
        if (where >= board_size)
            return Square (Piece::out_of_board, Color::none);
        return squares[where];
    }

    void init_classic_board(Board& b) {
        // Place pawns
        for(size_t i = 0; i < 8; ++i){
            b.set(31 /*col 1, 2nd row*/ + i, Square(Piece::pawn, Color::black));
            b.set(81 /*col 1, 7th row*/ + i, Square(Piece::pawn, Color::white));
        }

        // Place the rest but with s/kings/queens
        int w = 0;
        for( auto p : {Piece::rook_castle, Piece::knight, Piece::bishop, Piece::queen} ) {
            b.set(21+w, Square(p, Color::black));
            b.set(28 /*col 8, 1st row*/ - w, Square(p, Color::black));
            b.set(91 /*col 1, 8th row*/ + w, Square(p, Color::white));
            b.set(98 /*col 8, 8th row*/ - w, Square(p, Color::white));
            ++w;
        }

        // Place kings
        b.set(25, Square(Piece::king_castle, Color::black));
        b.set(95, Square(Piece::king_castle, Color::white));

        // Empty squares inbetween the pieces
        for(int x=0; x < 8; ++x)
            for(int y=0; y < 4; ++y)
                b.set(41+x+y*10,Square(Piece::none, Color::none));
    }

}

move.h

/*
    File: move.h

    Move utils and move history utils.
    Assumes that a history list and the board it's used with are consistent.

    Boardhistory:
    A move event consists of a series of board changes and a "move done",
    namely BoardChange.where == (boardsize = move_done), which is pushed last for each move event.

    Author: Ken Rouwas
    Date 2017-09-14
*/

#pragma once

#include <list>
#include <vector>

#include "board.h"

namespace Chess {

    const size_t move_done = board_size;

    struct Move {
        ssize_t from, to;
        Move(ssize_t, ssize_t);
        Move();
    };

    struct BoardChange {
        size_t where;
        Square old_square;
    };

    using Boardhistory = std::list<BoardChange>;
    using MoveSet = std::vector<Move>;

    void undo_move(Board&, Boardhistory&);

    /* Castling is identified by king move to its castling destination if permitted.
       Pawn promotion move deduced.
       All other moves are unconditional moves.
    */
    void do_move(Move, Board&, Boardhistory&, Piece pawn_promotion = Piece::queen);

    MoveSet valid_moves(Board&, Color turn); // This is the move generator

}

move.cpp

#include <iostream>
#include "move.h"

// Read move.h for specifications

namespace Chess {

    Move::Move(ssize_t _from, ssize_t _to) {
        from = _from;
        to = _to;
    }

    Move::Move(){}

    void undo_move(Board& b, Boardhistory& ml){
        if(!ml.size())
            return;
        if(ml.back().where == move_done)
            ml.pop_back();

        while(ml.size() && ml.back().where != move_done){
            b.set( ml.back().where, ml.back().old_square );
            ml.pop_back();
        }
    }

    static void do_change(Board& b, Boardhistory& bh, size_t where, Square new_square) {
        BoardChange change;
        change.old_square = b.get(where);
        change.where = where;
        bh.push_back(change);
        b.set(where, new_square);
    }

    void do_move(Move m, Board& b, Boardhistory& bh, Piece pawn_promotion){
        // Pawn promotion
        if(b.get(m.from).piece == Piece::pawn && m.to/10 == 2)
                do_change(b, bh, m.from, Square(pawn_promotion, Color::white));

        if(b.get(m.from).piece == Piece::pawn && m.to/10 == 9)
                do_change(b, bh, m.from, Square(pawn_promotion, Color::black));

        // Move rook if castling
        if(b.get(m.from).piece == Piece::king_castle && (m.from-m.to == 2 || m.from-m.to == -2)){
            if(m.to == 23){
                do_change(b, bh, 21, Square(Piece::none, Color::none));
                do_change(b, bh, 24, Square(Piece::rook, Color::black));
            }

            if(m.to == 27){
                do_change(b, bh, 28, Square(Piece::none, Color::none));
                do_change(b, bh, 26, Square(Piece::rook, Color::black));
            }

            if(m.to == 93){
                do_change(b, bh, 91, Square(Piece::none, Color::none));
                do_change(b, bh, 94, Square(Piece::rook, Color::white));
            }

            if(m.to == 97){
                do_change(b, bh, 98, Square(Piece::none, Color::none));
                do_change(b, bh, 96, Square(Piece::rook, Color::white));
            }
        }


        Piece pawn_replaced = b.get(m.to).piece;
        // Regular piece move
        do_change(b, bh, m.to, b.get(m.from));
        do_change(b, bh, m.from, Square(Piece::none, Color::none));


         // Pawn replaced empty square
        if(b.get(m.to).piece == Piece::pawn && pawn_replaced == Piece::none) {
            // En passant move
            if(b.get(m.from-1).piece == Piece::pawn_en_passant && (m.from-m.to == 11 || m.from-m.to == -9))
                do_change(b, bh, m.from-1, Square(Piece::none, Color::none));
            else if (b.get(m.from+1).piece == Piece::pawn_en_passant  && (m.from-m.to == 9 || m.from-m.to == -11))
                do_change(b, bh, m.from+1, Square(Piece::none, Color::none));
        }


        // clear flag of pawns with en passant potential
        for(size_t i=1; i < 9; ++i){
            if(b.get(50+i).piece == Piece::pawn_en_passant)
                do_change(b, bh, 50+i, Square(Piece::pawn, Color::black));
            if(b.get(60+i).piece == Piece::pawn_en_passant)
                do_change(b, bh, 60+i, Square(Piece::pawn, Color::white));
        }

        // Give two-square moved pawns en passant flag
        if(b.get(m.to).piece == Piece::pawn) {
            if(m.from/10 == 3 && m.to/10 == 5)
                do_change(b, bh, m.to, Square(Piece::pawn_en_passant, Color::black));

            if(m.from/10 == 8 && m.to/10 == 6)
                do_change(b, bh, m.to, Square(Piece::pawn_en_passant, Color::white));
        }

        // Lose castling potential
        if(b.get(m.to).piece == Piece::king_castle)
            do_change(b, bh, m.to, Square(Piece::king, b.get(m.to).piece_color));
        if(b.get(m.to).piece == Piece::rook_castle)
            do_change(b, bh, m.to, Square(Piece::rook, b.get(m.to).piece_color));

        BoardChange done;
        done.where = move_done;
        bh.push_back(done);
    }

    MoveSet valid_moves(Board& b, Color turn){
        MoveSet ret;
        Color enemy_color = (turn == Color::white) ? Color::black : Color::white;
        int pawn_dir = (turn == Color::white) ? -1 : 1;


        for(size_t from = 21 /*skip padding*/; from < 99 /*padding after this*/ ; ++from){
            if(b.get(from).piece_color == turn){
                switch(b.get(from).piece){
                    case Piece::king_castle:
                        if(from == 95 && b.get(96).piece == Piece::none && b.get(97).piece == Piece::none && b.get(98).piece == Piece::rook_castle)
                            ret.push_back(Move(from, 97));
                        if(from == 25 && b.get(26).piece == Piece::none && b.get(27).piece == Piece::none && b.get(28).piece == Piece::rook_castle)
                            ret.push_back(Move(from, 27));                  
                        if(from == 95 && b.get(94).piece == Piece::none && b.get(93).piece == Piece::none && b.get(92).piece == Piece::none && b.get(91).piece == Piece::rook_castle)
                            ret.push_back(Move(from, 93));
                        if(from == 25 && b.get(24).piece == Piece::none && b.get(23).piece == Piece::none && b.get(22).piece == Piece::none && b.get(21).piece == Piece::rook_castle)
                            ret.push_back(Move(from, 23));
                    // fallthrough
                    case Piece::king:
                        for(auto to : {from-11, from-10, from-9,from-1, from+1, from+9, from+10,from+11}) {
                            if(b.get(to).piece_color == turn || b.get(to).piece == Piece::out_of_board)
                                continue;
                            ret.push_back(Move(from, to));
                        }
                        break;
                    case Piece::pawn:
                        if(b.get(from+pawn_dir*11).piece_color == enemy_color)
                            ret.push_back(Move(from, from+pawn_dir*11));
                        if(b.get(from+pawn_dir*9).piece_color == enemy_color)
                            ret.push_back(Move(from, from+pawn_dir*9));
                        if(b.get(from+pawn_dir*10).piece == Piece::none)
                            ret.push_back(Move(from, from+pawn_dir*10));
                        if(b.get(from+pawn_dir*10).piece == Piece::none && b.get(from+pawn_dir*20).piece == Piece::none){
                            size_t row = from/10;
                            if((row == 3 && pawn_dir == 1) || (row == 8 && pawn_dir == -1))
                                ret.push_back(Move(from, from+pawn_dir*20));
                        }
                        if(b.get(from-1).piece == Piece::pawn_en_passant && pawn_dir == -1)
                            ret.push_back( Move(from, from-11) );
                        if(b.get(from+1).piece == Piece::pawn_en_passant && pawn_dir == -1)
                            ret.push_back( Move(from, from-9) );
                        if(b.get(from-1).piece == Piece::pawn_en_passant && pawn_dir == 1)
                            ret.push_back( Move(from, from+9) );
                        if(b.get(from+1).piece == Piece::pawn_en_passant && pawn_dir == 1)
                            ret.push_back( Move(from, from+11) );
                        break;
                    case Piece::knight:
                        for(auto to : {from+8, from+12, from+19, from+21, from-8, from-12, from-21, from-19}) {
                            if(b.get(to).piece_color == turn || b.get(to).piece == Piece::out_of_board)
                                continue;
                            ret.push_back(Move(from, to));
                        }
                        break;
                    case Piece::queen:
                    //fallthrough
                    case Piece::rook:
                    case Piece::rook_castle:
                        for(int dd : {1,-1,10,-10})
                            for(int d=dd; b.get(from+d).piece_color != b.get(from).piece_color && b.get(from+d).piece != Piece::out_of_board ;d+=dd) {
                                ret.push_back(Move(from, from+d));
                                if(b.get(from+d).piece != Piece::none)
                                    break;
                            }
                    if(b.get(from).piece != Piece::queen)
                        break;
                    case Piece::bishop:
                        for(int dd : {9,11,-9,-11})
                            for(int d=dd; b.get(from+d).piece_color != b.get(from).piece_color && b.get(from+d).piece != Piece::out_of_board ;d+=dd) {
                                ret.push_back(Move(from, from+d));
                                if(b.get(from+d).piece != Piece::none)
                                    break;
                            }
                        break;
                    default: // warnings unwanted
                    break;
                }
            }
        }
        return ret;
    }
}

main.cpp

/*
    // File: main.cpp

    TODO: don't allow moves that leaves king in check, and count check-mate as victory. Hence, perft will be off by a few.
    TODO: prompt pawn promotion, instead of the default queen.
    TODO: handle cases of no valid moves
*/

#include <iostream>
#include <ctime>
#include "board.h"
#include "move.h"
#include "gui.h"
#include "ai.h"

using namespace Chess;


unsigned long perft(Board &b, Boardhistory &h, int depth, Color turn) {
    turn = (turn == Color::white) ? Color::black : Color::white;
    if(!depth)
        return 1;
    int leafs = 0;
    for(Move m : valid_moves(b, turn)){
        if(b.get(m.to).piece == Piece::king || b.get(m.to).piece == Piece::king_castle){
            ++leafs;
            continue;
        }
        do_move(m,b,h);
        leafs += perft(b,h,depth-1,turn);
        undo_move(b,h);
    }
    return leafs;
}


int main(){
    std::cout<<"\nChesscpu 1.0\n\n";
    std::cout<<"Commands:\nyxyx: fromto move.\n0: regret move (last AI move will be reverted as well).\n1: change color (AI will make this move)\n2: exit.\n\n";
    Board b;
    Boardhistory h;
    init_classic_board(b);

    Color turn = Color::white;
    Color ai_color = Color::black;

    bool ai_has_king = true;
    bool human_has_king = true;


    if(false) {
        unsigned long t = time(0);
        std::cout<<"DEBUG: Perft(5) result (expecting 4897256): "<<perft(b,h,5,Color::black);
        t = time(0) - t;
        std::cout<<"\nTime "<<t<<"\n";
        return 0;
    }

    if(false){
        Move mv;
        unsigned long t = time(0);
        ai_move(b, h, turn, 7, mv);
        t = time(0) - t;
        std::cout<<"\nAI Time: "<<t<<"\n";
        return 0;
    }

    Move mv {0,0};
    while(ai_has_king && human_has_king){
        print_board(b, mv);
        if(turn == ai_color)
            ai_move(b, h, turn, 7, mv);
        else
            mv = read_move(valid_moves(b, turn), turn);

        if(mv.from == 0) {
            undo_move(b,h);
            undo_move(b,h);
            continue;
        } else if (mv.from == 1) {
            ai_color = ai_color == Color::white ? Color::black : Color::white;
            continue;
        } else if (mv.from == 2) {
            human_has_king = false;
            break;
        }

        do_move(mv, b, h);
        turn = turn == Color::white ? Color::black : Color::white;

        ai_has_king = human_has_king = false;
        for(size_t i = 21; i < 99; ++i) { // board.h about these magic numbers
            if(b.get(i).piece == Piece::king || b.get(i).piece == Piece::king_castle) {
                if(b.get(i).piece_color == ai_color) {
                    ai_has_king = true;
                } else {
                    human_has_king = true;
                }
            }
        }

    }
    print_board(b, mv);

    std::cout<<"\n\n";
    if(!human_has_king)
        std::cout<<"You lose.";
    if(!ai_has_king)
        std::cout<<"You win.";
    std::cout<<"\n\nBye!\n\n";
}

ai.h

/*
    File: ai.h

    Takes a board, returns an heuristically optimized move.

    Author: Ken Rouwas
    Date 2017-09-14
*/

#pragma once

#include "move.h"

namespace Chess {
    int ai_move(Board& b, Boardhistory& bh, Color turn, int depth, Move& _bm, int alpha = -400, int beta = 400);
}

ai.cpp

// See board.h on "magic numbers"
// File: ai.cpp

#include "ai.h"
#include <random>
#include <algorithm>
#include <iostream>


namespace Chess {
    std::mt19937 mt(time(0));


    void moveorder(MoveSet& ms) {
        std::random_shuffle(ms.begin(), ms.end());
    }

    static int evaluate_leaf(const Board& b) {
        int sum = 0;
        for(size_t i = 21; i < 99; ++i) {
            if(b.get(i).piece_color == Color::none)
                continue;
            int c = b.get(i).piece_color == Color::white ? 1 : -1;
            int v = 0;
            switch(b.get(i).piece){
                case Piece::pawn:
                case Piece::pawn_en_passant:
                    v = 1;
                    break;
                case Piece::rook:
                case Piece::rook_castle:
                    v = 5;
                    break;
                case Piece::knight:
                case Piece::bishop:
                    v = 3;
                    break;
                case Piece::queen:
                    v = 9;
                    break;
                default:
                break;
            }
            sum += c*v;
        }
        return sum;
    }

    static Color flipped_turn(Color turn) {
        if(turn == Color::white)
            return Color::black;
        return Color::white;
    }

    int ai_move(Board& b, Boardhistory& bh, Color turn, int depth, Move& _bm, int alpha, int beta) {
            /*
            MoveSet valid = valid_moves(b, turn);
            _bm = valid[mt()%valid.size()];
            return 0;
            */

        Move best_move;
        int best_score = turn == Color::white ? -300 : 300;
        if(!depth)
            return evaluate_leaf(b);

        MoveSet vmoves = valid_moves(b, turn);
        moveorder(vmoves);
                //int progress = 0; // Temporary hack to show progress
        for(Move m : vmoves){
                //if(depth == 8) // Temporary hack to show progress
                //  std::cout<<"\r"<<++progress<<"/"<<vmoves.size()<<std::flush;

            if(b.get(m.to).piece == Piece::king || b.get(m.to).piece == Piece::king_castle){
                best_score = turn==Color::white ? 200+depth : -200-depth;
                best_move = m;
                break;
            }

            do_move(m, b, bh);
            int new_score = ai_move(b, bh, flipped_turn(turn), depth-1, _bm, alpha, beta);
            undo_move(b,bh);

            if((turn == Color::white && new_score > best_score) || (turn == Color::black && new_score < best_score)){
                best_move = m;
                best_score = new_score;

                if(turn == Color::black) {
                    beta = new_score;
                    if(beta <= alpha)
                        break;
                }

                if(turn == Color::white) {
                    alpha = new_score;
                    if(alpha >= beta)
                        break;
                }
            }

        }
        _bm = best_move;
        return best_score;

    }
}

gui.h

/*
    File: gui.h

    Interactive graphical interface of a chess board.

    This is temporarily a quick and dirty solution.

    Author: Ken Rouwas
    Date 2017-09-15
*/

#pragma once

#include <vector>
#include "board.h"
#include "move.h"

namespace Chess {

    void print_board(const Board&, Move last_move);

    /* Returns when one of the provided valid moves is read */
    Move read_move(const MoveSet& valid_moves, Color turn);

}

gui.cpp

// File: gui.cpp
// This is quick, ugly, pragmatic, temporary.

#include <iostream>
#include <cctype>
#include <map>
#include "gui.h"



namespace Chess {

    using namespace std;

    static const std::map<Piece, char> sprites = { 
        { Piece::pawn,      'A' },
        { Piece::pawn_en_passant,   'P' },
        { Piece::rook,      'H' },
        { Piece::rook_castle,   'H' },
        { Piece::knight,        'F' },
        { Piece::bishop,        'I' },
        { Piece::king,      'K' },
        { Piece::king_castle,   'K' },
        { Piece::none,      '+' },
        { Piece::out_of_board,      '#' },
        { Piece::queen,         'Q' }
    };

    void print_board(const Board& b, Move last_move) {
        cout<<"\n   1 2 3 4 5 6 7 8";
        for(ssize_t i=20; i < (ssize_t)board_size; ++i){
            if(i%10 == 9)
                continue;
            if(i/10 == 10)
                break;
            if(i%10 == 0) {
                cout<<"\n "<<i/10<<" ";
                continue;
            }
            char s = sprites.at(b.get(i).piece);
            if(b.get(i).piece_color == Color::black)
                s = tolower(s);
            cout<<s;
            if(last_move.from == i || last_move.to == i)
                cout<<'<';
            else
                cout<<' ';          
        }
        cout<<"\n"<<endl;
    }

    Move read_move(const MoveSet& valid_moves, Color turn) {
        if(cin.fail()) {
            cin.clear();
            string dummy;
            cin >> dummy;
        }

        int in;
        Move ret;
        cout<<"Your move, "<<( turn == Color::white ? "white" : "black" )<<": ";
        cin>>in;

        // Command to undo 1 or 2 moves (2 to revert AI+own)
        if(in == 0 || in == 1 || in == 2){
            ret.from = in;
            return ret;
        }

        ret.to  = in%10+in/10%10*10;
        in /= 100;
        ret.from  = in%10+in/10%10*10;

        for(const auto m : valid_moves)
            if(m.from == ret.from && m.to == ret.to)
                return ret;
        cout<<"Invalid move\n";
        return read_move(valid_moves, turn);
    }

}
11 голосов | спросил chesscode20172017 18 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowMon, 18 Sep 2017 11:59:24 +0300 2017, 11:59:24

3 ответа


6

В целом этот код выглядит красиво, его легко читать и понимать, за исключением нескольких вещей.

Исключить магические числа

Единственное, что поражает меня, это то, что вы часто используете магические числа. Вместо этого используйте дескриптивные имена переменных, поскольку это затрудняет понимание кода в некоторых местах, например. здесь:

if (m.to == 23) { // why 23? What is its meaning?
    do_change(b, bh, 21, Square(Piece::none, Color::none)); // why 21?
    do_change(b, bh, 24, Square(Piece::rook, Color::black)); // why 24?
}

Я не согласен с объяснением, которое вы указали в файле board.h, который называет эти цифры и вычисляет их, будет грязным - все наоборот. Я, например, не нашел объяснения, приведенные в этом файле, удовлетворительными - мне не всегда ясно, что они означают и как они были получены. Вы должны избегать ситуации, когда кто-то должен прочитать код и попытаться выяснить значение значений. Чем меньше времени на понимание кода, тем лучше.

Я бы сказал, что вообще опасно предполагать, что что-то очевидно или не нуждается в объяснении «если вы знаете структуру» - это всегда тривиально для человека, разрабатывающего код, и не настолько тривиального для людей, пытающихся понять ваш способ делать вещи, включая вас из будущего.

Также идея дать людям подсказки в комментариях относительно того, где искать объяснение, является хрупкой. Очень вероятно, что вы измените свой код в будущем и забудете изменить комментарии, подобные этому. Кроме того, мне пришлось прочитать весь код, чтобы перейти к этому комментарию, и как бы я узнал только часть вашего проекта, что есть один комментарий в этом файле, который объясняет числа, используемые в нескольких других файлах?

Предпочитать возвращаемое значение по выходному параметру

Эта функция:

void init_classic_board(Board&);

должен возвращать Board вместо того, чтобы принимать его по параметру. Таким образом, более очевидно, что он делает. Это также проще в использовании, поскольку вы можете написать однострочный файл, инициализирующий его:

Board b = init_classic_board();

Современные компиляторы здесь будут использовать copy elision, поэтому нет ненужного копирования.

Мне кажется, что эта функция необходима для того, чтобы объект можно было использовать. Если я прав, подумайте о том, чтобы внедрить его как частный метод и вызвать его из конструктора. Если вы планируете добавить некоторые неклассические платы в будущем, вы можете передать перечисление конструктору, который будет запускать правильный метод init. На данный момент легко забыть вызвать эту функцию после объявления объекта - объект не должен находиться в унифицированном состоянии после вызова его конструктора. Задача конструктора - установить все переменные, а не какую-либо внешнюю функцию, которая может быть вызвана или нет.

Не помещайте столько кода в main.cpp, особенно игровой логический код

В идеале ваш main() должен содержать пару строк - таким образом ваш код легче управлять и понимать. Если бы вы разделили весь этот код на функции значимых имен, было бы легче выяснить, что делает код. Кроме того, избегайте определения функций в main.cpp. perft() принадлежит вашей логике игры и необходим для запуска игры независимо от пользовательского интерфейса, тогда как main.cpp сам по себе идет чтобы отличаться в зависимости от интерфейса, который вы собираетесь использовать в будущем. perft() (а также код из main ()) находится внутри Chess. Весь этот код логически связан и необходим для игры, поэтому естественным шагом было бы поместить его в некоторый модуль game_logic. То, что теперь находится внутри функции main(), должно быть делегировано в автономную функцию имени типа playGame() (в идеале после деления его на более мелкие вспомогательные функции).

Используйте более описательные имена

Названия функций, такие как perft(), не говорят о том, что должна делать эта функция.

Это не проблема, но я также думаюBoard::get()/set() следует переименовать в Board::getSquare()/setSquare() - это путь более ясный, что они делают.

Сделать Board::get() return const reference

Кажется, что вы используете этот метод только для сравнения его возвращаемого значения с чем-то, и вы никогда не копируете возвращаемое значение или не изменяете его, поэтому разумно устранить ненужное копирование, сделав возвращаемое значение const Square&

Отформатируйте свой код с удобочитаемостью

Это:

static const std::map<Piece, char> sprites = {
    { Piece::pawn,      '        A' },
    { Piece::pawn_en_passant,   'P' },
    { Piece::rook,              'H' },
    { Piece::rook_castle,       'H' },
    { Piece::knight,            'F' },
    { Piece::bishop,            'I' },
    { Piece::king,              'K' },
    { Piece::king_castle,       'K' },
    { Piece::none,              '+' },
    { Piece::out_of_board,      '#' },
    { Piece::queen,             'Q' }
};

намного проще, менее утомительно читать и находить возможные ошибки, чем это:

static const std::map<Piece, char> sprites = {
    { Piece::pawn,      'A' },
    { Piece::pawn_en_passant,   'P' },
    { Piece::rook,      'H' },
    { Piece::rook_castle,   'H' },
    { Piece::knight,        'F' },
    { Piece::bishop,        'I' },
    { Piece::king,      'K' },
    { Piece::king_castle,   'K' },
    { Piece::none,      '+' },
    { Piece::out_of_board,      '#' },
    { Piece::queen,         'Q' }
};
ответил KjMag 19 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowTue, 19 Sep 2017 22:39:05 +0300 2017, 22:39:05
2

Я хотел бы начать с того, что вы сделали очень хорошую работу по отделению логики от дисплея. Кроме того, для использования реальных типов для таких вещей, как Color и Piece. Это редко можно увидеть здесь!

Я согласен со всем, что KJMag сказал в их ответе. Вот несколько других вещей, которые я заметил.

Стиль именования

По большей части ваши имена довольно хороши. Однако я замечаю, что вы переключаетесь между CamelCase для имен типов и используете символы подчеркивания для переменных и констант. Это немного раздражает. Обычно вы придерживаетесь одного. CamelCase обычно использует заглавную букву для первой буквы имени типа и строчной буквы для первой буквы переменной или константы. Константы и enum s - это все кепки (не мои личные предпочтения) или начинаются с префикса - либо k (например, kKing, kPawn, kRook и т. Д.) Или аббревиатура имени типа (pKing, pPawn, pRook и т. Д.).

Избегайте конструкторов, которые не производят действительный объект

У вас есть конструктор для Square, который не принимает аргументов и создает квадрат с недопустимым piece и piece_color. В чем его цель? То же самое с конструктором для Move, который не принимает аргументов. Когда объект строится, он должен быть полностью построен. Сделав невозможным создание недопустимого объекта, вы устраните целый класс ошибок.

Использование 1D-массива в виде 2D-массива

Прекрасно использовать 1D-массив для представления 2D-объекта. Это может быть более эффективным. Но пользователи класса, которые делают это внутри, не должны знать об этом и выяснить, как с этим бороться. Например, функция init_classic_board() не должна проходить в 31 для представления 1-го столбца и 2-й строки. Он должен иметь возможность передать 1, 2. Выполнение этого, вероятно, облегчит устранение этих магических чисел!

Использовать больше пробелов

Функция valid_moves() очень трудно читать по разным причинам. Одна из этих причин заключается в том, что код переполнен. Я рекомендую использовать больше горизонтальных и вертикальных пробелов. Было бы неплохо увидеть пробелы до и после каждого оператора. Итак:

if(b.get(from+pawn_dir*11).piece_color == enemy_color)
    ret.push_back(Move(from, from+pawn_dir*11));

будет выглядеть так:

if(b.get(from + pawn_dir * 11).piece_color == enemy_color)
    ret.push_back(Move(from, from + pawn_dir * 11));

Я почти пропустил оператор + между from и pawn_dir. Я бы также добавил пробелы между каждым из операторов if. Вы можете утверждать, что это делает функцию намного дольше. Вы правы, и поэтому вы должны взять каждый из этих case операторов и поместить их в отдельные функции. Например, вы можете создать функцию valid_pawn_move().

Создание массива внутри предложения for - это что-то новое для меня. Это очень трудно читать и сбивать с толку. Я рекомендую просто написать массив перед for.

ответил user1118321 20 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowWed, 20 Sep 2017 08:32:56 +0300 2017, 08:32:56
1

Ваш код легко понять. Есть несколько вещей, которые мне нравятся:

  • Использование демистифицированных номеров для представления позиций платы. Я согласен с вами в том, что в них нет ничего волшебного.
  • Ваш выбор очевидных, самых простых имен для всех, над которыми вы работаете, например king, pawn.

Что бы я делал иначе:

  • Я попытался бы сделать a1 квадрат номер 11 вместо 21, так что вы можете использовать более натуральные числа (от 11 до 88), чтобы ссылаться на квадраты на доске.
  • История платы не должна называться ml; Я не понимаю это имя.

Еще несколько замечаний сверху вниз:

void Board::set(const size_t where, Square s) {
    if (where >= board_size)
        return;
    squares[where] = s;
}

Вместо того, чтобы возвращаться, вы должны выбросить исключение, поскольку попытка изменить панель за пределами ее границ явно является ошибкой программирования на плате 10x12. То же самое для Board::get.

Я бы ввел два конструктора для Chess::Square.

Параметры const size_t не полезны, поскольку size_t всегда передаются по значению. По крайней мере, в объявлениях следует указывать const. Однако в реализации это приемлемо.

В init_classic_board код для размещения фрагментов настолько сложный (сначала поставьте королеву, а затем замените ее королем), у вас также может быть только 16 одинаковых линий, каждая на одну часть. Или небольшая вспомогательная функция, использующая цвет и список из 8 штук, например,

place_pieces(91, Color::black,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);
fill_row(81, Color::black, Piece::pawn);
fill_row(71, Color::none, Piece::none);
fill_row(61, Color::none, Piece::none);
fill_row(51, Color::none, Piece::none);
fill_row(41, Color::none, Piece::none);
fill_row(31, Color::black, Piece::pawn);
place_pieces(21, Color::white,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);

Что касается фигурных скобок: всякий раз, когда внутренний цикл for требует фигурных скобок, я также помещаю скобки вокруг внешнего for. Я только опускаю фигурные скобки для однострочных и блочных операторов с выражением тела с одним слоем.

Ниже case Piece::rook_castle:, if имеет неправильный отступ. Вы должны позволить вашему IDE форматировать код для вас. Это также сделает пространство между ){ согласованным.

Перед case Piece::bishop: существует // fallthrough отсутствует.

После default:, break имеет неправильный отступ.

В read_move, cin>>in должен быть заключен в предложение if.

ответил Roland Illig 20 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowWed, 20 Sep 2017 09:14:46 +0300 2017, 09:14:46

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132