Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review from Laravel Daily youtube channel video sheet #2

Open
olbrichattila opened this issue Oct 19, 2022 · 1 comment
Open

Review from Laravel Daily youtube channel video sheet #2

olbrichattila opened this issue Oct 19, 2022 · 1 comment

Comments

@olbrichattila
Copy link

olbrichattila commented Oct 19, 2022

I think it is nice work!

First I would use a specific readme for the project, not the default Laravel one.

I would expect to use the repository pattern in larger more complex projects.

If you are using the Repository pattern.
What would happen if you implement a new repository using an existing interface not using eloquent, but for example calling another service API to fetch the data in the future?

I prefer not using complex return types like:

return $this->model->create($attributes);

but I would convert it into an array:
return $this->model->create($attributes)->toArray();

(same applies for lists, like ->all() or ->get(): I would use ->all()->toArray() to abstract out from the eloquent models.

I like using the array, so your repository is interchangeable and does proper abstraction from the Eloquent implementation.

I cannot see return types in your interfaces and repositories.
I think it is a good practice to use return types. It makes it more readable and more robust so the same function cannot return with multiple types. It also avoids some side effects.

I also prefer using strict_types which makes the core also more robust.

If you use PHP 8.> I prefer using the new PHP feature when injecting your repositories:

public function __construct(private readonly BrandContract $brandRepository) {}

then you don't have to define the protected $brandRepository;
For me, it makes the code shorter and more readable.

If you rather use
protected $brandRepository;
I would prefer to define the type/interface as well (also depending on the PHP version), you can use the doc-type comment for older PHP versions as well.

You have repeated validators, what if you would create a custom request for these repeating validators?

Missing tests

@anuzpandey
Copy link
Owner

Thank you for the review.
I will sure update the project with all the new things that I have learned from past couple of years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants