Peer review

Hi! Fun idea for a project. I unfortunately could not test the code, because following the instructions got me this error:

kbjakex:~$ choosenim devel
Downloading Nim latest-devel from GitHub
[##################################################] 100.0% 0kb/s
 Extracting linux_x64.tar.xz
 Extracting linux_x64.tar
    Setting up git repository
   Building Nim #devel
  Compiler: Already built
   Switched to Nim #devel
kbjakex:~$ cd temp
kbjakex:~/temp$ nimble install https://github.com/Cloudperry/the-witness-puzzle-maker

Downloading https://github.com/Cloudperry/the-witness-puzzle-maker using git
  Verifying dependencies for [email protected]
    Prompt: No local packages.json found, download it from internet? [y/N]
    Answer:  Installing [email protected]
    Prompt: cligen not found in any local packages.json, check internet for updated packages? [y/N]
y
    Answer: Downloading Official package list
    Success Package list downloaded.
       Tip: 6 messages have been suppressed, use --verbose to show them.
     Error: Package not found.

(This was on the fuksilaptop, which at the time of writing I no longer have access to due to forgetting my charger to the campus yesterday. I'm now on a Mac, and now the choosenim installer fails, seemingly because of C compilation errors, which is probably because the Mac is outdated in every way imaginable (13 years old). Sorry I wasn't able to test this.)

Having browsed the code, it's pretty clear you've programmed before. Not a whole lot to say.

• Code coverage is great!
• Code is certainly readable (at least with my Rust/C/C++/Java background), but also,
• I appreciate the comments for the more obscure syntactical features, such as * exporting to other modules. in retrospect I wish I'd done this as well. I'd still perhaps appreciate more comments on fields such as those in Level (pointGraph/cellGraph/pointData/cellData?) though, to get the basic idea
• Code has been split up appropriately to multiple files
• If there's one thing I'm a bit uncertain it's the Graph structure.
  • The genericity itself isn't an issue, but it's being parameterized with float, Point2D and even seq, and the generic type is used as a key to a hash table. Floats are typically a poor choice for a hash map key because of floating-point imprecision (e.g. the typical 0.1 + 0.2 != 0.3 example), and seq as an array is heavy to hash / compare for equality (and in this case it's even a seq of Point2Ds). If at all possible, consider if you can find a way to use integers as keys (sometimes you can generate unique ids for objects and use those as keys, but in this case, since the problem as I understand it is largely on a grid, can you use a grid index?)
  • If keys were indeed to be flattened to ints, you could try to reduce the heavy nested hash collection structure to seq[seq[N]] or similar, assuming your keys are dense. I admit my understanding of the codebase is not good enough to be able to tell if this is viable, though.
• For fast insertions / removals when order doesn't matter, instead of a hash set, consider using a "bag"? Bag as in basically seq, but instead of an order-preserving remove(), you instead remove by swapping the to-be-removed element with the last element, and then remove the last element, which is a trivial O(1) operation. Hash sets are memory-heavy and are a poor fit for iteration over the data, and it seems a fast contains() isn't necessarily needed here? Bags are dense in memory (good iteration performance), have no memory overhead and have O(1) insert/remove
• Raylib 👍

I had only heard of Nim until now, and I certainly enjoyed peeking into this world. The syntax is quite elegant in many ways, very concise without sacrificing legibility. It's unfortunate my codebase leaves such a bad impression of Rust, in contrast.

Last up: you've done a lot, but seems like you have a fair amount to do still, especially on the editor front. Not too many weeks left, so best of luck. Certainly more ambitious a project than mine.

(Project was downloaded on Fri, October 7th at 1:30 PM)

Hello @Cloudperry!

I was excited to open your project since I've played The Witness before.
This was a cool opportunity to see some Nim, since I've only heard of it previously!

Environment

I am using Zorin OS (Ubuntu clone) and the latest Nim installation according to your guide.
It seems to be working!

I downloaded the project on the 13th of October at 11:18.

I used the latest git commit 9be1573.

Usability

What did I do?

• Read through the docs
• Go through Linux installation
• Build project
• Run game
  • When not setting the level as a cli argument it doesn't seem to open with testLevel / testLevel.bin -> seems that it needs a full path to the file
  • Also the program screws a bit with my screen settings: it set my resolution at 1280x720 at the lowest refresh rate possible 😂
• Run editor
• Start going through code

Experience

Setup was really smooth and running the program went quite well.

The main problem right now (even though you've done a lot) is that there isn't much functionality.

Personally, to avoid a crunch, I would cut some stuff out to focus on the most important features in terms of the course for now:

• leave the editor in the shadow and focus on the game and checking algorithms
• implement the most interesting tiles like Squares and Blocks, but forget about stuff like Jacks

Code

I was unable to find parts which to criticise taking into consideration the previous peer review.

• I liked the NIMNOTE stuff to get more into Nim
• Project structure is good
• Code is really well commented and readable
• Consistent with Nim style guide
• Test coverage is good
• Unit tests are good

Summary

The code is really good! However, the current state of the app is really unfinished: I hope that you can get at least some functionality done before the end of the course. Good luck!