@june23 There are some thing that I wouldn't do.
Never leave code that you commented out in you code.
//$path = $request->file('image')->store('images', 'public');
Not have any methods in your controller that are not part of the rest full
- index
- create
- show
- update
- store
- edit
- delete/destroy
public function getProductsByCategory(string $category_name): JsonResponse {
public function detail(string $id): JsonResponse {
For validation, consider using FormRequests instead of inline validation.
$validated_data = $request->validate([
'name' => 'required|string|max:255',
'slug' => 'required|string|max:255',
'description' => 'required|string|max:255',
'price' => 'required|numeric',
'sku' => 'required|string|max:255',
'stock_quantity' => 'required|numeric',
]);
Use the id, and not the name of the category when searching by category
public function getProductsByCategory(string $category_name): JsonResponse {
$product = $this->productService->getProductsByCategory($category_name);
return response()->json([
'products' => $product,
]);
}
I guess you are using some kind of UUID as your primary keys, but I would suggest that you use regular unsigned ints, since they are easier to index, and that makes your application faster.
public function delete(string $id): JsonResponse {
I see that you are using a service class for some things, and while that in it self isn't wrong, the way you go about it is too wordy.
public function store(array $data, string $category_id): Product {
$product_to_save = new Product([
'name' => $data['name'],
'slug' => $data['slug'],
'description' => $data['description'],
'price' => $data['price'],
'sku' => $data['sku'],
'stock_quantity' => $data['stock_quantity'],
]);
$category = Category::findOrFail($category_id);
$product = $category->products()->save($product_to_save);
return $product;
}
The category id should already be in the data passed in through your validation, and since you have all fields fillable you can basically just do
public function store(array $data): Product {
return new Product($data);
}
this one can also be simplified
public function getProductsByCategory(string $category_name): Collection {
$category = Category::where('name', $category_name)->firstOrFail();
return $category->products()->with('category')->get();
}
public function getProductsByCategory(int $categoryId): Collection {
returnProducts::where('category_id', $categoryId)->get();
}
Here you really should use pagination, more that a 25-50 records in your table will decrease performance, and increase rendering times. You should basically never use all() unless you know that you have a set amount of records.
public function index(): Collection {
$product = Product::all();
if ($product->isEmpty()) {
throw (new ModelNotFoundException)->setModel(Product::class);
}
return $product;
}
This looks dangerous to me. My guess here is that this method is used when a new shipment of the product has arrived, or when a shipment has been sent to the customer. The code then requires that you always use the full stock when you send it to the database, instead of just using the amount received or shipped.
public function updateProductStock(string $id, int $amount): bool {
$product = Product::findOrFail($id);
if($amount < 0) {
return false;
} else {
$product->stock_quantity = $amount; //Change this to $product->stock_qty += $amount;
$product->save();
return true;
}
I would also consider using route model binding instead of fetching the data as a additional step.
public function detail(string $id): JsonResponse {
$product = $this->productService->detail($id);
return response()->json([
'product' => $product,
]);
}
Could instead be
public function detail(Product $product): JsonResponse {
return response()->json([
'product' => $product,
]);
}
You can also inject the Productservice, rather than creating it in the constructor.
public function __construct(ProductService $productService) {
$this->productService = $productService;
}
Could be
public function store(ProductRequest $request, ProductService $productService): JsonResponse {
Your code isn't that bad in general, but it has room for improvements.
Final thoughts.
Use camelcase for all you methods and variables, and only use snakecase for database tables and column names.
With proper validation, you can use
protected $guarded = [];
instead of adding all columns to the fillable array.
Run you code through tools like Laravel Pint, Rector and PHPStan(Larastan) to improve the quality.
Good luck :)