A m,n,k engine in C++.
ristinolla's Introduction
ristinolla's People
ristinolla's Issues
Code Review 1
Hello @altarchess!
Now a disclaimer right up front: I've maybe read 5 lines of C++ before this review, so I can't go into immense detail on your implementation, but anyways, here's my shot at a code review for your Tic-Tac-Toe -engine:
Environment
With small changes to
GUI/gui/config.cfg
: change player_0 path to ../Engine/RistiNolla
GUI/gui/config.py
: change "\config.cfg"
to "/config.cfg"
I got your project running on Linux!
- Linux, Zorin OS (Ubuntu clone)
- Date and time of download: 5.10. 14:13
- Python 3.10.7
I tested 2 versions of your program, the latest commit (4caf64c) and the last commit for the 4th week (093cd0f).
Usability
What did I do?
- Clone project
- Build the Engine executable
- Install python dependencies for the GUI
- Fix compatibility errors
- Play a bit against the AI with the default settings
- Change the Board config to size 4x4 and max_depth to 6
- Here the AI got stuck on their second move
- Play around with different configs and comparing the performance with different max_depth values
- Start reviewing code
Experience
The engine seems to be working well with this config:
- size_x: 12
- size_y: 12
- max_depth: 6
After a bit of playing though, the AI freezes up: I get tons of "Failure to print bestmove!" errors on the console.
It also seems possible to play for the AI if you click fast enough on the GUI.
With small boards the AI doesn't really know what to do on their first/second move.
Code
- The project structure looks really good to me. I especially love how the GUI is completely separated from the Engine itself.
- Looking at the actual search -algorithm:
- It's interesting how it's implemented as a negaMax with makeMove & undoMove
- Not sure if it's more optimal to just reserve more memory with instances of the board as opposed to "undoing" the move after calling negaMax. (This is a tiny detail though)
- I think the search could be optimized by moving the board evaluation out from the makeMove function (evalChange), because right now it evaluates the board after every move at every depth, but it only needs to be evaluated at the leaves of the game tree.
- It's interesting how it's implemented as a negaMax with makeMove & undoMove
- More documentation comments would help
- Especially in the
comms.cpp
file: clarify the IO structure and more docs on the individual blocks of code
- Especially in the
- The board class looks really good and optimized.
- I recommend implementing unit tests and performance tests asap: unit tests for making sure your code works as inteded and performance tests for monitoring engine performance during optimization.
- The GUI code also looks clean and well structured.
Summary
Your project needs testing, more documentation and there is still some room optimization (it seems you're already working on transposition tables). Also, the AI freezes I experienced could just be because the project is under construction or because of my environment.
Overall, the project looks really nice. Good job!
Code review 2
Hey! Cool project. Didn't expect to see someone casually using prefetch(), but you clearly have a bit of a history with optimizing AI opponents...
The project was downloaded at 7:35 PM @ October 15th and tested on fuksilaptop 2021 (running Ubuntu 20.04).
I started by reading the design document, after which I read the user guide and built the engine & GUI and started the game. Aside from having to install a newer Python (not your problem), build process was smooth.
Either I found a difficult combo and it hung, or a board this big just takes a long while, but it didn't respond even after at least a minute of waiting with these inputs. Tested 5x5 grid as well (because previous reviewer had issues with small boards) and that definitely works, plenty fast too, and I was not able to beat the AI. Also, it starts spamming the console once there are no moves to make, but I assume this is known.
To the code: I have some C++ background, so I was hoping to be able to give feedback here, but aside from pedantic suggestions I wasn't actually able to find much to nag about, ...
... so here's some pedantry...
... and I'm sure there are counterarguments to many of them so take them as you will!
- Generally, constexpr variables are preferred over
#define
s, unless you necessarily want to have them configurable at compile-time without a fancier system... std::from_chars
is preferred overstoi
, because stoi is technically locale-dependent and possibly slower.- Prefixing C functions (such as stoi) with
std::
is encouraged since they are technically not guaranteed to exist without the std:: prefix, even if in practice they do on the major compilers (GCC, Clang, MSVC). - Some allocations could be avoided with the use of
std::string_view
overstd::string
- for example thesplit()
function could operate entirely on string views. Of course, in this case performance literally does not matter, so the effort is probably not worth it. std::vector::at()
does bounds checking, which isn't necessary for example here incomms.cpp
.- Functions that aren't meant to be called from outside a specific .cpp file shouldn't be declared in a header file; for example
processCommand
seems like it should be internal tocomms.cpp
. Prefer forward-declaring it there if necessary, and make it static to ensure internal linkage - board.h has #defines before header guards
- You have a fair bit of global state. Since this is single-threaded, it's not much of an issue, but generally not having global state is preferred where possible
The code is fairly C-like at a times, but in this case I see zero issues with that. Memory is not being manually managed and C++ is used to make life easier as appropriate, and everything does look to be (mostly) valid C++, unlike a lot of C-like-C++ codebases I've seen ๐
Code quality wise my main concern would be primarily the lack of testing, but also the naming: I'd appreciate either unshortened names or comments explaining the abbreviations somewhere (what's "comms" short for, or especially "tt"?). Variables like Board *b
, SearchData *sd
could use longer names too, although the meaning of these is easier to infer.
This is all still very surface-level unfortunately, but I couldn't find any deeper issues. ...so, looking good! Aside from the freeze I got, this seems presentable already. Nice work!
Edit: Forked the project on accident, please don't mind that!
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.