Am i following good practices?
Hey, i was wondering if anyone with experience could tell me if i am following good practices when doing custom actions, thx
Solution:Jump to solution
```php
private function findLicense(string $key): ?License
{
return License::with('product')->where('key', $key)->first();
}...
10 Replies
Please read #✅┊rules before posting your code and try to explain what is your question
I just wanted to know if I was writing decent code, that’s it :(
share your code, not image
this is the reason i did not do it, will try to send it in multiple messages tho
protected function createReedemAction(): Actions\Action
{
return Actions\Action::make('redeem')
->label('Redeem license')
->action(function (array $data)
{
$currentUser = Auth::user()->id;
$license = License::where('key', $data['key'])->first();
$oldSubscription = Subscription::where(
'product_id', $license->product_id)
->where('user_id', $currentUser)
->first();
if (!is_null($oldSubscription))
{
$oldSubscription->end_date = Carbon::parse(
$oldSubscription->end_date)
->addDays($license->expiration_time);
$oldSubscription->update();
$license->delete();
Notification::make()
->title('Subscription updated')
->success()
->body('Existing subscription updated.')
->send();
return;
}
protected function createReedemAction(): Actions\Action
{
return Actions\Action::make('redeem')
->label('Redeem license')
->action(function (array $data)
{
$currentUser = Auth::user()->id;
$license = License::where('key', $data['key'])->first();
$oldSubscription = Subscription::where(
'product_id', $license->product_id)
->where('user_id', $currentUser)
->first();
if (!is_null($oldSubscription))
{
$oldSubscription->end_date = Carbon::parse(
$oldSubscription->end_date)
->addDays($license->expiration_time);
$oldSubscription->update();
$license->delete();
Notification::make()
->title('Subscription updated')
->success()
->body('Existing subscription updated.')
->send();
return;
}
$subscription = new Subscription();
$subscription->start_date = now();
$subscription->user_id = $currentUser;
$subscription->product_id = $license->product_id;
$subscription->end_date = now()->addDays(
$license->expiration_time);
if ($license->product->status
=== ProductStatus::Offline->value)
{
$subscription->paused_at = now();
}
$subscription->save();
if (!is_null($subscription))
{
$license->delete();
Notification::make()
->title('Subscription redeemed.')
->success()
->send();
$this->redirect(
$this->getResource()::getUrl('index'));
}
else
{
Notification::make()
->title('Error happened at redeeming.')
->danger()
->send();
}
})
->form([
Forms\Components\TextInput::make('key')
->label('Key')
->required()
->rules([new \App\Rules\License()]),
]);
}
$subscription = new Subscription();
$subscription->start_date = now();
$subscription->user_id = $currentUser;
$subscription->product_id = $license->product_id;
$subscription->end_date = now()->addDays(
$license->expiration_time);
if ($license->product->status
=== ProductStatus::Offline->value)
{
$subscription->paused_at = now();
}
$subscription->save();
if (!is_null($subscription))
{
$license->delete();
Notification::make()
->title('Subscription redeemed.')
->success()
->send();
$this->redirect(
$this->getResource()::getUrl('index'));
}
else
{
Notification::make()
->title('Error happened at redeeming.')
->danger()
->send();
}
})
->form([
Forms\Components\TextInput::make('key')
->label('Key')
->required()
->rules([new \App\Rules\License()]),
]);
}
FYI, you can post longer code as a gist on your GitHub and just provide the link.
This is an opinionated answer but I have refactored your code to make it follow "best practices" in Laravel/PHP:
protected function createReedemAction(): Actions\Action
{
return Actions\Action::make('redeem')
->label('Redeem license')
->action(function (array $data): void {
$currentUser = Auth::user()->id;
$license = $this->findLicense($data['key']);
if ($license === null) {
$this->sendNotification('Invalid License Key', 'danger');
return;
}
DB::transaction(function () use ($license, $currentUser): void {
$oldSubscription = $this->findOldSubscription($license->product_id, $currentUser);
if ($oldSubscription) {
$this->updateOldSubscription($oldSubscription, $license);
$this->deleteLicenseAndNotify($license, 'Existing subscription updated.');
} else {
$this->createNewSubscription($license, $currentUser);
}
});
})
->form([
Forms\Components\TextInput::make('key')
->label('Key')
->required()
->rules([new \App\Rules\License()]),
]);
}
protected function createReedemAction(): Actions\Action
{
return Actions\Action::make('redeem')
->label('Redeem license')
->action(function (array $data): void {
$currentUser = Auth::user()->id;
$license = $this->findLicense($data['key']);
if ($license === null) {
$this->sendNotification('Invalid License Key', 'danger');
return;
}
DB::transaction(function () use ($license, $currentUser): void {
$oldSubscription = $this->findOldSubscription($license->product_id, $currentUser);
if ($oldSubscription) {
$this->updateOldSubscription($oldSubscription, $license);
$this->deleteLicenseAndNotify($license, 'Existing subscription updated.');
} else {
$this->createNewSubscription($license, $currentUser);
}
});
})
->form([
Forms\Components\TextInput::make('key')
->label('Key')
->required()
->rules([new \App\Rules\License()]),
]);
}
Solution
private function findLicense(string $key): ?License
{
return License::with('product')->where('key', $key)->first();
}
private function sendNotification(string $message, string $type = 'success'): void
{
Notification::make()
->title($message)
->status($type)
->send();
}
private function findOldSubscription(int $productId, int $userId): ?Subscription
{
return Subscription::where('product_id', $productId)
->where('user_id', $userId)
->first();
}
private function updateOldSubscription(Subscription $oldSubscription, License $license): void
{
$oldSubscription->end_date = Carbon::parse($oldSubscription->end_date)
->addDays($license->expiration_time);
$oldSubscription->update();
}
private function createNewSubscription(License $license, int $userId): void
{
$subscription = new Subscription();
$subscription->start_date = now();
$subscription->user_id = $userId;
$subscription->product_id = $license->product_id;
$subscription->end_date = now()->addDays($license->expiration_time);
if ($license->product->status === ProductStatus::Offline->value) { // could use just ProductStatus::Offline here instead of chaining ->value
$subscription->paused_at = now();
}
if ($subscription->save()) {
$this->deleteLicenseAndNotify($license, 'Subscription redeemed.');
$this->redirect($this->getResource()::getUrl('index'));
} else {
$this->sendNotification('Error happened at redeeming.', 'danger');
}
}
private function deleteLicenseAndNotify(License $license, string $message): void
{
$license->delete();
$this->sendNotification($message);
}
private function findLicense(string $key): ?License
{
return License::with('product')->where('key', $key)->first();
}
private function sendNotification(string $message, string $type = 'success'): void
{
Notification::make()
->title($message)
->status($type)
->send();
}
private function findOldSubscription(int $productId, int $userId): ?Subscription
{
return Subscription::where('product_id', $productId)
->where('user_id', $userId)
->first();
}
private function updateOldSubscription(Subscription $oldSubscription, License $license): void
{
$oldSubscription->end_date = Carbon::parse($oldSubscription->end_date)
->addDays($license->expiration_time);
$oldSubscription->update();
}
private function createNewSubscription(License $license, int $userId): void
{
$subscription = new Subscription();
$subscription->start_date = now();
$subscription->user_id = $userId;
$subscription->product_id = $license->product_id;
$subscription->end_date = now()->addDays($license->expiration_time);
if ($license->product->status === ProductStatus::Offline->value) { // could use just ProductStatus::Offline here instead of chaining ->value
$subscription->paused_at = now();
}
if ($subscription->save()) {
$this->deleteLicenseAndNotify($license, 'Subscription redeemed.');
$this->redirect($this->getResource()::getUrl('index'));
} else {
$this->sendNotification('Error happened at redeeming.', 'danger');
}
}
private function deleteLicenseAndNotify(License $license, string $message): void
{
$license->delete();
$this->sendNotification($message);
}
thanks andrew! will check it out in a min