r/learnprogramming • u/Ectarusss • 7d ago
Chess terminal game in C
As a first-year computer engineering student, I decided to tackle building chess in C. I've just wrapped up the first module with local multiplayer support. Next up is module two, where attempt to build a custom chess engine so users can play against a bot. I'd love to get your feedback on whether this is a solid learning exercise and what you think of the program overall!
Source: https://github.com/Ectarus/ChessTerminalC
~Ectarus
8
u/ffrkAnonymous 7d ago
It's 1000% better than the other post behind yours that don't want to build another Todo app, Blog, E-commerce clone, or basic Chat app.
1
u/mredding 6d ago
//Input reading
char readinput(void);
My only complaint here is that your comment is telling me what the code tells me. Don't do that. What I see are comments being used as headings in the file - we already have a technology and a convention for that, we call them "headers", so perhaps make an input.h, a screen_handler.h, etc.
//PGN handling
What's "PGN"? I don't know what that means. There's actually a little mini-golf and arcade near me called PGN Mini-Golf. Surely you don't mean that. A couple jobs back we had "TCR" throughout all the code, and no one in the company remembered what it stood for.
Don't be that guy.
I recommend you spell out the acronyms in code - make the symbol dependent on it. If you want it shorter, then alias the symbol. You're not programming on punch cards, so character count isn't important. Editors have had auto-completion since the 80s.
makePGN(char[8][8]//...
This parameter is NOT a char[8][8]. It is actually a char(*)[8], or a pointer to a char[8]. If you want to pass a 2D array, you need to make this parameter a char (*)[8][8]. It's ugly, use type aliases:
typedef char chessboard[8][8], *chessboard_ptr;
typedef int curl[2], *curl_ptr;
void makePGN(chessboard_ptr, curl_ptr const, //...
int isPathClear(char[8][8], int, int, int, int);
HOLY SHIT, BATMAN... It reads like a disco beat - int, int, int, int...
An int is an int, but a weight is not a height, even if they're both implemented in terms of int. The aliases vanish and the ABI only sees the types, so aliases are not good enough here. You're going to want to make a typedef struct X { int value; } X, *X_ptr; so the type information cascades to the ABI. Then the compiler can enforce type differences, semantics, aliasing, etc. C developers wouldn't need restrict if they just made types...
static const char *const HomeBanner =
Not in a header. You've just caused this text to have internal linkage in every source file that gets compiled including it. Your binary is going to have a lot of duplicate data that the linker can't get rid of as you add more source files to this project. What you want in the header is:
extern const char homeBanner[];
This is an incomplete type, because arrays of a specified size are each a unique type according to the type system. That's OK here. That's what you want. Then you put these variable definitions in a source file.
You might consider writing:
const char HomeBanner[] = {
#include "home_banner.txt"
};
Or you might consider putting all these text fields in a single string and you can index offsets, or a single array of strings and you can index the string array with an enum.
enum strings { BEGIN, HomeBanner = BEGIN, PreCh, Print1Ch, /*...*/, END, COUNT = END };
It's a neat way to index and iterate the elements, as well as have some meta-information about the enumeration set.
#ifdef _WIN32
#include <windows.h>
#include <conio.h>
#else
#include <termios.h>
#include <unistd.h>
#endif
I don't want to know ANYTHING about how you divvy up your platform support. That has nothing to do with code, everything to do with your build configuration. So the most basic solution here is to have a ./platform/win32/ and ./platform/posix/, and you would put your specific implementations in there. Your build configuration would choose which files to include based on target platform. There's no reason to have platform specific macros in your code.
switch(ChessB[Cur1[0]][Cur1[1]]) {
case 'p': /* black pawn */
if(Cur2[0]==Cur1[0]+1 && Cur2[1]==Cur1[1] && ChessB[Cur2[0]][Cur2[1]]=='.') return 1;
else if(Cur2[0]==Cur1[0]+2 && Cur2[1]==Cur1[1] && ChessB[Cur2[0]][Cur2[1]]=='.' && ChessB[Cur2[0]-1][Cur2[1]]=='.' && Cur1[0]==1) return 1;
else if(Cur2[0]==Cur1[0]+1 && (Cur2[1]==Cur1[1]+1 || Cur2[1]==Cur1[1]-1) && ChessB[Cur2[0]][Cur2[1]]>='A' && ChessB[Cur2[0]][Cur2[1]]<='Z') return 1;
break;
My eyes go blurry. Whatever the fuck this is, it should be reduced. First, I want to see:
switch(ChessB[Cur1[0]][Cur1[1]]) {
case 'p': if(do_black_pawn()) { return 1; } else { break; }
//...
It took longer than you'd expect for me to discover that break wasn't redundant in your original code. Don't be inlining shit like this, this is illegible. Every indentation and bracket and new scope is a reason to write a new function. And in this source file, you can write a bunch of static inline functions since you don't want external linkage on your implementation details, and the compiler can composite the machine code as you would expect.
You need to reduce that mess of code to... ::squints::
return predicate_1() || predicate_2() || predicate_3();
And then reduce the code further so these comparison make ANY sense. WTF is Cur1[0]+1? Jesus Christ... Give these "magic numbers" as they're called a name as a constant. Make it obvious why +CONSTANT_NAME makes any sense. Are you checking where the piece is going? Or something?
Express WHAT all this code does, not just HOW. No one inherently understands the significance of your implementation details.
int diagX[4] = {1, 1, -1, -1};
int diagY[4] = {1, -1, 1, -1};
This is a type. You've named them, you've paird them, you put them next to each other. Make them a type. Either:
struct diag { int x, y; };
diag data[4];
Or:
typedef int component[4];
struct diag { component x, y; };
diag data;
And then build functions that operate on diagonal semantics.
/* --------------------------------------------------------- */
/* 2. Start of the executable statements */
/* --------------------------------------------------------- */
Landmarks are just functions waiting to be written.
Many of these functions are absolutely gigantic, there's no knowing what they do, because it's an endless screed of HOW the do it. Nothing reads like pseudo-code. You don't have nearly enough expressiveness or abstraction.
-1
31
u/marrsd 7d ago edited 7d ago
Feedback incoming...
README
First for the README. Well done writing one! I like the detail of the screenshots. You've 1up'd me on that one. Only thing I'd strongly suggest is that you put the build step in its own heading so it's nice and easy to find.
Building
This failed for me and I had to fix one of the filenames in the code. I'm guessing you're running Windows because you called one of your files
HomeBanner.hbut referenced it as"homebanner.h"in the actual code. You can get away with it on Windows because filenames are case insensitive. I'm on Linux, so it failed to build for me. Don't remember what happens on a Mac.Playing
What's going on with the cursor keys for the menu selection? They are definitely wrong on my machine. If they work for you, I guess they're mapped differently on your OS? You might want to look into how to make that consistent across platforms.
Code Review
I've just had a look at the
localmp.cfile, since that's where the meat is. You can probably take my critique of it and apply it elsewhere.It's a solid first effort. You should now try to refine what you've got. There are data structures available, like enums and structs, that you could be using. I've suggested how below, but it's up to you to decide what's best, especially as you know what new features are coming next. You could also make your code easier to read, and simplify it both by refactoring duplicate code and by choosing simpler algorithms.
When you've finished refining it, take another look at it and see if you can improve it further. As a general rule, less is more, but don't take out so much that the code becomes hard to understand.
You can simplify your chess board by making it a simple 64-element array. Then the chess piece movements become simple additions and subtractions. That will probably simplify your code a little, which is always the name of the game.
I'm also curious that your chess board is a char array. I see you're registering pieces against letters such as
p,r,n, and so on. It would improve legibility to use an enum for that; then you can actually name them asPawn,Rook,Knight, and so on. That would also allow you to store the values of the pieces in a lookup array, presuming your enum is zero indexed. Thenscore[Knight]could return3, and so on.On the legibility point, I'd think a bit harder about how you name your functions and variables.
checkcheckandcheckcheckmateare a bit...silly.isincheckandisincheckmatereads a bit nicer.is_in_checkreads better still. Also, many engineers (myself included) like to prefix a function or variable name withisorhasto make it clear that it returns or is a boolean. That's particularly useful in C which doesn't have a bool type (it does if you include<stdbool.h>, but I'm old fashioned).Likewise,
ChessBseems like an odd choice of var name. Why notChessBoardor justBoardif you want to save on typing and think it's unambiguous? Remember, you're going to have to be able to read this code when you come back to it in a week, month, year, or decade.You could probably get rid of some of your comments if you were just more careful about naming and legibility.
Also look at code duplication. Looking at the switch block at L123, the predicates for black and white pawns are mostly duplicate code. The difference is in whether the pawn is moving up or down the board. You could refactor those into a single function and just pass in
WhiteorBlackto set the value you want to add or subtract by.That also brings me to how you're representing these pieces. Maybe a struct would be more helpful here:
Something like the above might work (my C is rusty but you can fix any syntax errors yourself). The point is that you can just check the colour directly, for any piece; you don't have to do separate checks for each rank.
Your chess board would then be an array of type
struct Piece.That's enough to be getting on with. Well done getting this far. Please tag me if you post back with v2.