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 master 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:

• Laravel Passport used for handling authorization! Nice!
• Database seeders are present!
• FormRequests are used to validate incoming requests? Nice de-composition of logic 🧠👌

Interesting points:

• Definitely not a complaint, but just a suggestion: did you know that instead of $this->hasMany('App\Service');, you can also write $this->hasMany(Service::class);, provided that you import the relevant class? I like that approach more, it eliminates string-typing and possibly allows IDEs and editors to make smarter suggestions
• I can’t exactly put it into words or explain it clearly, but the code feels in some places… well, compared to the second group where every place was a joy to read and understand intuitively nearly immediately, here, one has to think for a while. I think some of the parts are not a stereotypical Laravel code, that could be the reason. But I’m not entirely sure. I’m typing this point as I’m going through the AuthController, FYI. I guess the best I can describe it is with an image, but I’m really sorry for not being able to explain better:

• UserController:L58-60 is somewhat amusing 😅

Bad points:

• No test coverage makes your co-workers sad 😭
• PSR-2 code style guide is often ignored 😢
• The default welcome template is still present both as a template file, and as a route. That doesn’t seem to be necessary 🤔
• There’s sometimes unused code left in methods, like in ServiceController:L30
• Sorry, I’m not quite clear on as to why assignments such as UserController:L36 or :L47 should be needed – you should be able to get the correct entity just by using route model binding, shouldn’t you? And it should also fail for you automatically if the entity doesn’t exist unless some misconfiguration happened.
• There should be a better way to update models with request data – in simpler cases, properly setting $fillable should do the job. In more complex cases, additional methods on models should be implemented, rather than cluttering controllers with unnecessary code.
• Speaking of controller code, many of the methods present in controllers should be elsewhere. I’m talking about the data-fetching methods, such as ServiceController::getServicesByDate, ::getServicesByCategory and ::searchServicesByKeyword, among others. I would probably not go for repositories for a project of this size, but I’d at least put this into models, if not somewhere else.

[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. In many places, the code felt cluttered, or it felt like it doesn’t belong to the place in which it is currently present. In other places, a simply duplicated/copy-pasted code was found.

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: 13/20

[nathaniel-ideyatech]

Hi @ml-milan-vit , thanks for reviewing our project. I've learned many things just from reading your review. Your insights are much appreciated, and I laughed when I saw your meme. Have a good day!