Small code review - Easter break

[AlessioVerardo]

Pawnies Code review

Hello team ! As promised during our last meeting, we do a small code review of your code base.
We were really impressed by the code you were able to produce. It is clean, consistent and uses a lot of new technologies (Jetpack-Compose and Kotlin), this means that we have not much to say.

With your team, we can see that the code review process is really working and makes the code really really good. The small comments we will do mostly concern chess and missing comments but not on the implementation details.

General Remark

Code structure

• The package structure is relatively well organized. We were just wondering if the package mobile.state should be moved in mobile.ui or not.

Code style

• You use a lot of Kotlin features, making the core more readable.
• All the conventions you chose to apply at the start of the project make the code very understanble and consistent. Congratulations for that.
• The classes and functions are well commented.
• You could consider using enum values for the collection and field names instead of hardcoded string (e.g. games, users,whiteId, blackid, ...). By doing this, you avoid magic number(s) and potential mistakes in the code by writing the value again.

By file

Only files for which we have a comment are referenced here.

Package mobile.application.authentication

AuthenticatedUser

• Maybe you can just rename the firestore variable into store to avoid confusion.
• The parameters of the class are not commented.

Package mobile.application.chess

ChessFacade

• You could consider using enum values for the collection instead of hardcoded string (e.g. games, users,whiteId, blackid, ...)
• private data class StoreMatch not commented.

Package mobile.application.chess.engine

Board

• Missing comment for @return parameter on function set. Moreover, you could add a comment explaining that if the piece is null, it is deleted from the broad.

Color

• Missing comment for the function denormalize.

Package mobile.application.chess.engine.rules

Moves

• (Regarding TODO in line 204) Just to remind you, in the castling functions the position that should not be in check are not the same as the one that have to be empty. We advise you to take only the king's direction as an argument, as the rest is either a position known at the start (kingStart) or positions that can be calculated from this direction (empty = all positions where the rook passes, check = all positions where the king passes). For us, this seems better than hardcoded positions.
• We also think that should avoid magic number such as 3 line 239 in the code and use them as constant.

Package mobile.application.chess.engine.implementation

PersistentGame

• Missing comment for some @param of the constructor.

Package mobile.application.social

• In the search method, make sure not to query the database each time a character is entered as this will quickyl make you run out of Firestore resources.

Package mobile.infrastructure.persistence.store

Query

• Missing comment for @return parameter on function limit.

Package mobile.infrastructure.state and mobile.infrastructure.ui

We didn't check in full details how these two packages work since they consist of graphical stuff and it seems that you are more aware about how this works than us.

Tests

Your tests look very good, kudos for that

Conclusion

Your code is very good overall, you have nice abstractions and lots of efforts put in. The documentation and maning are clear and understable, the codestyle is consistent. The app design is very cool. Keep up the good work!