Code Review 2

[sofiak-hel]

Context

Some context that might be very relevant to understanding my review, is that I've actually worked with JavaScript using JSDoc and Typescript as helping tools, using Node.js professionally for years. This means I have quite a lot of experience with Javascript/Typescript and Node, but I might be making assumptions about pure typescript and Deno. I have looked into using Deno at work too instead of Node, but I'm not too familiar with it.

I also apologize that I do have quite a bit to say, but do keep in mind that I have formed somewhat strict mindsets about typescript in my years of coding and overall I think the codebase looks pretty good!

Documentation and first glance

• When doing the initial registering, Lichess seems to make you check a few boxes that imply that I'm breaking quite a few ToS rules while doing this:

  1. I agree that I at no time receive assistance during my games (from a chess computer, boo, database or another person)

  2. I agree that I will not create multiple accounts (except for the reasons stated in the Terms of Service)

  Now I do personally understand that apparently to create a bot account, you do need to first create a fresh account, so logically obviously I would not be breaking ToS while doing this, but you might want to add a mention for that.

• The linked lichess guide seems to want me to create a config.yaml file, and is in general a little confusing and does not seem to completely match this project, you might want to simply make a direct reference to the wanted bit of guide instead.
• Seems the issue with the previous review (#1) still applies, nowhere a mention that a game should already be running. I also have absolutely no experience with lichess, so it's not exactly easy to figure out what I needed to do to get the program running properly.
• The UI for the terminal application while the bot is running is pretty nifty though, fun to look at.
• At least my first attemt in a game ended with the bot winning, and it wasn't able to resolve itself and got stuck in "Waiting for player move...". Not great for user comprehension.
• deno coverage as stated in testausdokumentti.md does not work out of the box. Apparently I would need to specify a list of files.
• When testing I noticed that of my 24 CPU threads only one was used at a time. Now usually this might not be a problem, but I also noticed that the depth-searching does take quite a while to run. You might want to look into implementing the minimax alpha-beta-pruning utilizing multithreading. I'm not too familiar with deno, but at least node has capable tools for building multithreaded applications using Worker Threads. From what I understand the algorithm is possible to implement multithreadedly at the cost of understandability, but I would at least try to see what kind of performance improvement it could have.

Moving on to reading and reviewing the actual code

• First thing I noticed that my vscode is complaining about some quite common Typescript issues, such as imports ending with .ts (allowImportingTsExtensions), which is against regular typescript conventions and some issues with typing here. The latter error seems to be caused by events-array being not typed and having obj being not typed. One thing I try to tell my more junior coworkers, when writing typescript/typed javascript it is best to avoid using implicit any's, and actually to enforce this I would recommend having noImplicitAny enabled in the typescript configuration to enforce this, so implicit any's are easily missed.
• This brings me to my second point, having a tsconfig.json would be highly beneficial, as you could through that actually control what kinds of typescript rules you want to use (or not use, ie allowImportingTsExtensions as mentioned above). Here's a guide for adding a tsconfig.json for Deno-projects: https://deno.com/manual@v1.32.5/advanced/typescript/configuration. I believe Deno comes with some of it's own formatting tooling and whatnot, but utilizing all of the formatting and linting tools in your disposal will usually make things easier for both yourself and the people reading your code.
• One thing I actually stumbled across very recently at work too, is that in situations like this you should still stick to using type (type alias), if the typing is not meant specifically for extending, which this type at least does not seem to be. The difference between type and interface are very small, but can cause issues down the line: https://github.com/microsoft/TypeScript/issues/15300
• I feel like this code is split into too many functions in some places. For example the game loop could I think very well just be inside the run function. A lot of programmers seem to have a "fear of long functions" but I think it's somewhat unwarranted. A function is fine even if it's a few hundreds of lines long as long as it's straightforward and doesn't have too much indentation. If the function does a lot of going back-and-forward or very deep indentations, those are good situations where to split functions into smaller components. Otherwise for readability I find that it's better to put "as much linear algorithmic code as possible" into one function¹. Having functions split this much makes this code for me a little bit hard to follow.
  ¹: the above exception of course, where function is too indented or goes back-and-forth too much.
• The same goes for files, having small files that have next to no logic makes the project again a little bit hard to follow. An easy example to give is src/ai/chess/moves/directions.ts. I honestly feel a little bit like combining all the code in the whole moves-folder might actually be not that bad, most of the files in that folder are less than 100 lines long. If the files are designed to be longer eventually, that's somewhat fair enough, but personally I would keep files merged until they need to be split, rather than the other way around.
• Something I noticed very early actually, is that in many cases you seem to have the same variables/parameters doubly-defined. By this I mean the types are defined both in-line and in the comments, which at least seems redundant, and in many (like this linked one) inconsistent and incorrect. I tend to prefer implicit typings at work, when said types are available, because that means less places where something breaks when you change a type in one place, because the implicit typings will be propagated along the pipeline. One place I do however tend to use explicit types even when implicit types are available, is sometimes in return values (at least in public API) to make sure that the return values are always the same, and breaking changes in return values are spotted early.
• Something I've noticed is that you tend to use a lot of arrow functions for defining regular functions, such as here. I would recommend defining these functions simply with function keyword as I personally think it makes the code a bit more readable. I think arrow-functions are great for situations where you need a closure so you don't need to use Function.prototype.bind, but in other places where the function is meant to be used regularly in-place I think the function keyword makes the intent of the functions more clear. Defining them as arrow-functions into const variables makes it seem, that the functions are indeed meant to be variables that simply contain functions, instead of just named functions, which is semantically slightly different and I think it's a somewhat important diffrence.
• I notice that in a quite a lot of places you tend to use raw string values such as w, b, e1, etc., such as here, and the thing that I start to wonder if Enums would do a better job at minimizing "magic numbers". I understand that for positions in the board, like e1 it might not be efficient, but I somewhat think, that especially since you seem to a bit of conversion in how the board positions are represented, I think it might be better to simply represent them as Enums.

In conclusion

In conclusion I would say that at least in a quick glance this codebase seems pretty alright, although there are quite a few changes (at least in semantics) that I would personally make. Good job on this project, the program is certainly fun to watch as it goes, and good luck going forward!

[Keskimaki]

Thank you for a very comprehensive review!

Better explanation about setting up a Lichess bot account is probably in order. The current experience of starting a game and there being no indication about the game ending are certainly not ideal and something I would like to improve. I intended for the bot to be capable of automatically accepting challenges as well, but the implementation of the more specific chess rules like castling, en passant etc. took longer than I expected.

Multithreading is something that I didn't even think about, but would make a lot of sense as a future improvement.

I come from a mostly JavaScript/Node-background and wanted to experiment with new, but also somewhat familiar technologies with this project. Deno differs from Node in having a native TypeScript support so there is no need for a separate tsconfig.json, unless you want to override some of Deno's mostly sensible defaults. For example noImplicitAny is enabled by default and I have simply overriden the rule in a few places. This was done mostly for speed of development.

Having .ts at the end of imports is the correct convention in Deno, although it feels somewhat wrong for me as well. Deno has inbuilt linting and formatting tools which replace the more typical ESLint and Prettier setups in Node.

I agree with the suggestions on typing. The current types are a bit of a mess as I lack experience with TypeScript and initially used the chess.js library for most of the game logic. The Lichess API requires using UCI notation, but I would otherwise have only used array indexes instead of a mix of them and UCI board positions.

The double definitions are an unfortunate end result of me not being sure what kind of documentation is expected for the project. I typically go with the also not ideal "the code is the documentation"-approach so I might have gone overboard with pointless comments this time.

When it comes to arrow functions, I personally feel like it's simpler to just use the same function notation for both regular and lambda functions etc. but using the function keyword for regular functions is probably the more widespread opinion.