mukama's avatar

API RESOURCE CODEBASE REVIEW

I would like to ask someone to take a look at my code (API ) and share your review based on your experience in coding.

https://github.com/chisumo2016/-library-management-api

1 like
4 replies
martinbean's avatar

@mukama Had a quick look. Few notes:

  • Code in general looks alright.
  • Don’t comment out code. Just delete it. If you need to refer back to it, that’s the entire point of version control: you can just look at a previous commit where that code existed. Commenting out code just makes it messy.
  • This is unnecessary: https://github.com/chisumo2016/-library-management-api/blob/9f9aa16ab72641d0d0942a99ec2218dc8068acf3/app/Http/Controllers/API/BookController.php#L66-L78. If you are using route–model binding and the requested model does not exist, Laravel will automatically return a 404 response. You don’t need to do it yourself. You’re also catching all exceptions, so if there’s an issue that’s something else (and not the model not existing) you’re never going to know because you just catch it and return a 404 response, regardless of what the exception actually was (it might have been something completely unrelated).
  • Please do not roll your own authentication and return plain text tokens. Use one of Laravel’s many, many first-party packages for authentication. It has two specifically for API authentication: Sanctum (and API tokens) and Passport (for OAuth 2, an actual authorisation standard).
  • I don’t know what you’re doing here, but that’s not how you use attributes in PHP.
1 like
JussiMannisto's avatar

Please do not roll your own authentication and return plain text tokens. Use one of Laravel’s many, many first-party packages for authentication. It has two specifically for API authentication: Sanctum (and API tokens) and Passport (for OAuth 2, an actual authorisation standard).

They're not rolling their own authentication, they're using Sanctum, which uses bearer token authentication. The unhashed token is sent to the client while the server retains the hash digest. It looks ok to me.

1 like
JussiMannisto's avatar

A couple of things right off the bat:

  • The project uses Laravel's default README page.
  • The project is named "laravel/laravel" and the description is "The skeleton application for the Laravel framework."
  • The repository name starts with a dash, which isn't normal.

If you don't add any documentation, people will have no idea what the project is about. They have to start going through the project file-by-file, trying to guess what you were trying to do.

Edit. I now see that it's about physical library books. When I saw "Library" and "API" in the name, my programmer brain assumed it's about software libraries. You should add at least a short project description as a courtesy to the reader.

1 like
vincent15000's avatar

Hmmm ... why don't you protect the API controllers functions with policies ?

Please or to participate in this conversation.