Feedback on C++ application

[chesscoder201717]

Hello, I'm new on this forum and I don't know how to post code. Nothing happens when I press the <code> button.

I have some code that I hope I can get some help with to make more professional, it's an excercise that I've given myself..

I have the code uploaded already on stackexchange but the discussion takes place here obviously. Also now noticed that he code won't fit on a single post either, it's 18k chars but not that big conceptually.

https://codereview.stackexchange.com/questions/175946/chess-in-c-honest-feedback

If I knew what you should be looking for then I wouldn't have to ask :p. So I hope you people can criticize me into a better programmer.

Thanks in advance.

[48T7M4Gy]

When you press the <> button in the small Format: toolbox on the right you post your code between the two <...> here </...> tags.

Your code is very long so post it in separate chunks.

Do you have any specific areas of you program that you want addressed?

[chesscoder201717]

@kemort, nothing specific. I don't know what's important to look at here. Perhaps the overall impression or if there's something specific that anyone reacts to. I don't know what to look for.

[Little Captain]

One thing I noticed that you can improve on is separating the struct and enum types into a separate header file: MiscTypes.h or HelperTypes.h?

Right now you have them all in a Board.h file and all the method definitions in a Board.cpp file. Put the major class and its definitions into its own pair of header/source files and then move the smaller enum and struct types into their own sets of files.

And this is just me but I rarely use #pragma once as that's more of a compiler-supported command (though a lot of compilers DO support it nowadays). I typically go with the more traditional #ifndef guards. Some will disagree... :)

I didn't go through the logic for the actual game, I must admit :p Unless there's something specific in that area you're looking for?

Hope I helped,
Joe
[e-mail]
www.concordspark.com

[chesscoder201717]

Not directed at anyone special: any notice on the code quality? Something felt horribly wrong writing that code, but I can't put my finger on it. I wrote a chess engine before that, but it was more elegant, and I got in some nice lambdas there to make the code small and clear/expressive.


Little Captaion, I sense some Java in you, or am I completely wrong? You remind me some of my java teacher.

#pragma once is fine nowadays. I've yet seen anyone disagree, and it allows for some optimization apparently (can just skip the file instead of reading to the end of the file to #endif), that's what I've heard.

Typically you'd want to collect relevant stuff in in the same file. "MiscTypes.h" doesn't say much. The idea is that chess pieces belongs to the board and is defined together with the board. You could give pieces their own header, that wouldn't be wrong. But there's something nice about having the pieces with the board on the same place imo, they kind of belong and are part of the same. The color I figured are part of the board too, they describe the color of a piece.

But I just realized now what you might have been thinking of. 1 file (h+cpp), 1 class, is that the idea? I'm not so sure that's idiomatic or has a purpose or is very interesting, but I'm still learning.

Yes, your comment helps very much, love the perspective, that's important. Thank you.

[Thomas1965]

Why didn't you create an abstract class for the chess peace with concrete classes for rooks, pawns..?
What's the point of color none?
Why doesn't the constructor of Board init the board instead of of a separate function init_classic_board(Board&); ?

Now you have many files like Little Captain suggested but it's quite a hassle to download and built the project.

[chesscoder201717]

@Thomas

The board is "initialized" but it's empty. init_classic_board places pieces according to classical chess, it's a utility function that has "nothing" to do with the board in general, it's just a setup.

Color::none isn't really necessary to be honest. I used it to avoid reading the wrong color, better no color at all than the wrong one. To lessen the risk of bugs. But in hindsight it do look suspicious. But a Piece::none with color white seemed a little odd semantically.

I'm not sure what benefit there is of making classes for the pieces?

[JLBorges]

> "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.

Why should it become messy?
Wouldn't avoiding these magic numbers make the code more readable, more maintainable?

For instance, with:

struct board
{
    static constexpr std::size_t N = 8 ; // 8x8 (playing area)
    static constexpr std::size_t B = 1 ; // border size (on each of the four sides)
    // ...
};

struct position
{
    std::size_t row ; // row 1 == first row
    std::size_t col ; // col 1 == first col

    constexpr operator std::size_t () const
    { return ( row + board::B ) * ( board::N + board::B*2 ) + col ; }
};

Instead of:

// 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));
}

something like this:

// Place pawns
for( size_t col = 1; col <= board::N; ++col ) { // for each of the N cols

        b.set( { 2, col }, Square(Piece::pawn, Color::black) ) ; // second row
        b.set( { board::N-1, col }, Square(Piece::pawn, Color::white) ) ; // second last row
}

Haven't looked at the rest of the code; will do so if get (quite) some time to spare.

[Thomas1965]

Normally a piece has a value (pawn = 1, knight and bishop = 3...) for the board evaluation and
Where would you want to store it?
If you want to create a GUI a piece need to have a bitmap.
Where would you want to store it?

Just an idea:

const int ROWS = 8;
const int COLS = 8;

enum PieceType
{
  PAWN, BISHOP, KNIGHT, ROOK, QUEEN, KING
};

enum Color
{
  WHITE, BLACK
};

class ChessPiece
{
public:
  ChessPiece(PieceType type, int value, Color color): 
    mType(type), mValue(value), mColor(color)
  {}
  int getValue() {return mValue;}
  Color getColor() {return mColor;}
  PieceType getType() {return mType;}
private:
  int mValue;
  Color mColor;
  PieceType mType;
};

class ChessBoard
{
public:
  ChessBoard()
  {
    mBoard.resize(ROWS * COLS);
    // create all the pieces and put on starting position
  }
private:
  vector<unique_ptr<ChessPiece>> mBoard;
};

[jonnin]

if you are looking at chess there is a standard engine format so that board programs and engine programs can communicate plug and play style.

[Little Captain]

chesscoder201717

I do some Java programming, but C++ and C# are my main tools :)

The only reason I suggested moving the smaller types to a separate file is that the file(s) you had were Board.h and Board.cpp.

If I were a 3rd party programmer who stepped in to help, I wouldn't expect the enum types, etc... to be inside a file called Board.cpp. Again, if you read my previous post, MiscTypes.h was just a suggestion. There are more appropriate names that I haven't thought of and you probably will :)

For #pragma once, while it is widely supported, it is non-standard and does have its own drawbacks:
https://stackoverflow.com/questions/787533/is-pragma-once-a-safe-include-guard

Again, if it suits your purpose, feel free to use it. Coding can be an art sometimes, and if the tools fit your vision without causing problems, feel free to use it.

Cheers!
Joe
[e-mail]
www.concordspark.com

[chesscoder201717]

Captain,

#pragma is now standard and you're linking to a 8.5 years old post.

What you're suggesting with the header files simply isn't modern C++ idiom. You typically pack things that belong into the same .h file unless they're optional utils or of such size and complexity that it's better to put it elsewhere.

[mbozzi]

The specific pragma
#pragma once
Is supported on basically all mainstream compilers. However, some preprocessors may recognize and optimize for handling either one, none, or both of include guards and #pragma once .

You typically pack things that belong into the same .h file

Interface granularity is especially appreciated in large projects. Segregating components helps manage compile-time dependencies. (Boost does this well, for a real-world example.)

IMO, this is less a question of idiomaticity and more a question of the design of the project in question and the opinion of the devs.

[Little Captain]

Thanks mbozzi for the clarification!

chesscoder201717 I don't have time to really find a recent post that explains all the ins and outs of #pragma. If you feel like using it, (and a lot of devs do) feel free to do so.

I still recommend at least researching on #pragma in your spare time.

Cheers!

Joe

[48T7M4Gy]

http://en.cppreference.com/w/cpp/preprocessor/impl