Code review

[lrollet1]

RockPaperScissors

Overall good job! The structure of your code is easy to understand and the use of Mockito helps a lot with your testing. There is however a concerning lack of documentation and you should work on it ASAP. Furthermore, the code style is not consistent and lots of part would heavily benefit from spacing.

We found the following comments:

Your Repository is too bloated. Functionalities such as users, friends, games, statistics, ... could be split into different repositories to make the codebase easier to manage and re-use.

• Breaking/refactor codebase #129

All of your public functions should be documented (except maybe the really obvious ones). This principle applies especially to the interfaces and abstractions. Most of your code is not documented at all and it heavily hinders its comprehension.

• Docs/firebase #130
• Document cache #134
• Document the ML part #135
• Document the rest #136

There is a problem with the code organization within your files and classes. Functions, variables and objects are mixed together randomly. Try to apply the following guideline: constants first, then fields/properties, then companion objects, then functions and event listeners. An example is the OfflineGameService.kt file.

• Breaking/refactor codebase #129

Your code is hard to read because you don't follow common layout principles. We see a lot of comments with no spacing after the //, some spaces missing before/after brackets or types, ... Don't forget that Android Studio has an automatic code formatter that you should run on the project before you commit. It is present everywhere, but a perfect example is the Cache.kt file.

• Breaking/refactor codebase #129

You have a tendency to drop extremely dense chunks of code in your functions without any spacing or dinstinction between the lines. Even inside a same function adding spacing between different parts of the code helps. An example is the TicTacToeGame.kt file.

• Breaking/refactor codebase #129
• @leonardopennino is there more to do ?

Some of your tests don't have explicit names. The test name should describe the functionality tested.

• Make sure cache tests have proper names @leonardopennino #137
• Make sure firebase tests have proper names @gaetschwartz #138
• Make sure statistics & leaderboard tests have proper names @panadax #139
• Make sure friendlist tests have proper names @bhvrgav #140
• Makre sure the ML part tests have proper names @aladin6060 #141

We are not sure what your LogService is used for.

Use it to log.

Your model package seems a bit heavy, you could split your model files in different subpackages (e.g. user, friend, game, ...)

• Breaking/refactor codebase #129

Make sure that your app doesn't crash when offline. For example the Leaderboard crashes the app when in airplane mode.

• Mostly fixed in Better nav + cache improvement #131

You shoudn't hard-code strings in your code, use strings.xml instead.

• Some done in Breaking/refactor codebase #129 and Better nav + cache improvement #131
• More todo @panadax @bhvrgav @leonardopennino

Check the warnings in Android Studio (Code > Inspect code...), you don't need to fix everything but there are a lot of easy things to fix. There are also a lot of warnings by the Android linter here: https://github.com/SDP-Rock-Paper-Scissors/RockPaperScissors/runs/6319295076

• Some false positives like typos, but important.