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 (Vertaisarviointi #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/[email protected]/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:
  Index signature is missing in type (only on interfaces, not on type alias) microsoft/TypeScript#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.