PHP code review

[ml-milan-vit]

Hello, thank you so much for your efforts! 👏

Throughout the day, I’ll be going through the code repository and gathering my thoughts here. For the record, I was going through the dev branch as that one seems to contain the most recent commits. The final score will be issued after evaluating projects from other groups, to keep the excitement in the air 🤣

Good points:

• In LoginController, I like that the JSON keys are stored as constants. I think that if the number of returned fields grew significantly, using an associative array would be better to reduce the number of constants, but here I kind of like it as it is.
• Nice usage of RegistrationFormRequest! Although, it could also include a custom method to directly return an instance of new User with the data populated, couldn’t it? It would look cleaner, as opposed to storing the conversion logic in the controller.
• Good job on properly specifying fillable fields in models!

Interesting points:

• Interesting choice of the library to implement JWT token generation… it looks easy to use, but why go for a third-party library when there is Laravel Passport? Although admittedly, Passport’s documentation can be a bit scary 😅
• In UserController, the getUsers method seems to return all users… That might be fine in this assignment, but don’t forget about pagination in the real projects! Other similar methods also lack pagination…
• In UserController, am I correct to assume that any user can delete any other user? I don’t see any sort of role-checking, am I missing something? 🤔 I mean, there are user roles in this project for sure, but I can’t find any sort of middleware that would actually check them 🤔

Bad points:

• No test coverage 😢
• In LoginController, it seems that $loginAfterSignUp variable controls the flow. It would’ve been better to control it via an environmental variable (env variable ➡️ config ➡️ controller) so that the flow can be easily changed by just adjusting the env variable, without a need for a code-level change.
• Whenever possible, I’d switch print-based logging for Laravel’s Log facade so that it can be easily silenced, highlighted or centrally redirected to a proper channel. I think it would be useful to get into the mind-set of always using Log, even for temporary debugging purposes.
• UserController combines spaces, tabs (L46-48), empty lines with white-space (L61), white-space at the end of the line (L58), say whaaaat 😱 Sorry, I’m crazy about proper formatting… Speaking of which, some of the lines in UserController go against the PSR-2 formatting guide, like line L55…
• UserController’s get and update methods could be using route model binding for simpler and more readable code. I think the same improvement could be used elsewhere as well.

[ml-milan-vit]

Additional points:

Bad points:

• I don’t think migrations properly set foreign keys 😢 Nor do some migrations follow Laravel’s recommended naming schema for migrations.

[ml-milan-vit]

Final score:

Thank you so much for your efforts! I hope the project was as fun for you to make as it was for me to review.

I think there are things that could be improved, both from a stylistic point of view, as well as the architectural point of view. Please don’t misunderstand me, I think this was a valiant first step in the wonderful world of Laravel (and a dreadful world of PHP 🤣), but there are definitely things that I’d wish to see improved. May I recommend watching tutorials at Laracasts website, as well as perhaps looking at other projects that were parts of this study program? Getting inspired by one another is a wonderful thing, after all 🤩

Score: 9/20

[mlph-iong]

Hi! Apologies for the late response. Please don't worry! We appreciate your honest feedback and will work on the points you shared. Thank you very much for taking the time :)