I'm using PHP/Laravel and I have two controllers.
- Controllers/PaymentController
- Controllers/PaymentExpressControler
They both create an request which is exactly the same, but it has some differences.
Let's say, they both use createRequest(Request $Request) which is a POST request to a third-party api.
I'm wondering how can I reduce the code, because it's clearly duplicated.
I was thinking of two options, a trait or a class.
- Controllers/Payment/(NameOfThirdParty).php which is a class with createRequest
How would this sound like?
My current createRequest
protected function createRequest(Request $request)
{
$this->cartCollection();
$availableMethods = ['mb', 'mbw', 'cc', 'dd'];
$tkey = uniqid(Auth::id(), true);
if (in_array($request->type, $availableMethods)) {
$payment_body = [
"method" => $request->type,
"value" => (float)\Cart::getTotal(),
"currency" => "EUR",
"key" => $tkey,
"type" => "sale",
"customer" =>
[
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "",
],
"capture" =>
[
"descriptive" => "Order",
"transaction_key" => $tkey,
],
];
if ($request->type == 'dd') {
$sdd_mandate = ['sdd_mandate' => [
"iban" => $request->iban,
"account_holder" => Auth::user()->name,
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "0000000",
"country_code" => "EU",
]];
$payment_body = array_merge($payment_body, $sdd_mandate);
}
$response = Http::withHeaders([
'AccountId' => config('payment.accountId'),
'ApiKey' => config('payment.apiKey'),
'Content-Type' => 'application/json',
])->post('REMOVED API LINK', $payment_body);
$this->addToPreOrdersTable($tkey);
return $response->collect();
}
}
}
protected function addToPreOrdersTable(string $hash)
{
$this->cartCollection();
OrderService::createPreOrder($this->contents, $hash, 'buy');
}
addToPreOrdersTable Is also in both controllers.
protected function index(Request $request)
{
if (isset($request->type)) {
$result = $this->createRequest($request);
$status = $result['status'];
if ($status == 'ok') {
$type = $result['method']['type'];
switch ($type) {
case 'cc':
return redirect($result['method']['url']);
case 'mb':
return view('payment', ['type' => 'mb', 'data' => $result['method']]);
case 'mbw':
return view('payment', ['type' => 'mbw']);
case 'dd':
return view('payment', ['type' => 'dd', 'data' => $result['method']]);
}
}
return back()->withInput()->withErrors(['Error' => 'An error has occured.']);
}
return view('payment');
}
3 Answers 3
Let's implements following: separate logic, refactoring of methods
Separate logic
Validate Requests/PaymentRequest.php add rule for validation
use Illuminate\Foundation\Http\FormRequest;
class PaymentRequest extends FormRequest
{
public function rules(): array
{
return [
'name' => ['required', 'string', 'in:cc,mb,mbw,dd'],
'telm' => ['string'],
'iban' => ['required', 'string']
];
}
}
take out the logic and leave the thin controller:
Controllers/PaymentController
public function index(PaymentRequest $request, PaymentService $service) {
return $service->run($request);
}
Now we are separating to service and external service. ExternalService - external request, service -our logic;
add Services/PaymentService.php and change method createRequest to send
class PaymentService implements PaymentServiceInterface {
private $externalPaymentService;
public function __construct(ExternalPaymentService $externalPaymentService)
{
$this->externalPaymentService = $externalPaymentService;
}
public function run(PaymentRequest $request) {
// Validate rule in PaymentRequest will not let you go further
// if (isset($request->type)) {
$tkey = uniqid(Auth::id(), true);
$result = $this->externalPaymentService->send($request, $tkey);
$this->addToPreOrdersTable($tkey);
$status = $result['status'];
if ($status == 'ok') {
$type = $result['method']['type'];
switch ($type) {
case 'cc':
return redirect($result['method']['url']);
case 'mb':
return view('payment', ['type' => 'mb', 'data' => $result['method']]);
case 'mbw':
return view('payment', ['type' => 'mbw']);
case 'dd':
return view('payment', ['type' => 'dd', 'data' => $result['method']]);
}
}
return back()->withInput()->withErrors(['Error' => 'An error has occured.']);
//}
// return view('payment');
}
protected function addToPreOrdersTable(string $hash)
{
$this->cartCollection();
OrderService::createPreOrder($this->contents, $hash, 'buy');
}
}
Services/PaymentServiceInterface.php
interface PaymentServiceInterface {
run(Request $request);
}
ExternalService/ExternalPaymentService.php
class ExternalPaymentService {
public function send(Request $request, $tkey)
{
$this->cartCollection();
$availableMethods = ['mb', 'mbw', 'cc', 'dd'];
if (in_array($request->type, $availableMethods)) {
$payment_body = [
"method" => $request->type,
"value" => (float)\Cart::getTotal(),
"currency" => "EUR",
"key" => $tkey,
"type" => "sale",
"customer" =>
[
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "",
],
"capture" =>
[
"descriptive" => "Order",
"transaction_key" => $tkey,
],
];
if ($request->type == 'dd') {
$sdd_mandate = ['sdd_mandate' => [
"iban" => $request->iban,
"account_holder" => Auth::user()->name,
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "0000000",
"country_code" => "EU",
]];
$payment_body = array_merge($payment_body, $sdd_mandate);
}
$response = Http::withHeaders([
'AccountId' => config('payment.accountId'),
'ApiKey' => config('payment.apiKey'),
'Content-Type' => 'application/json',
])->post('REMOVED API LINK', $payment_body);
return $response->collect();
}
}
}
Okay, we made first level, separated logic!
Refactoring of methods
Let's create Services/PaymentsRenderStrategy.php and will changing run in PaymentService
Services/PaymentsRenderStrategy.php
class PaymentsRenderStrategy {
private function cc($method) {
return redirect($method['url']);
}
private function mb($method) {
return view('payment', ['type' => 'mb', 'data' => $method]);
}
private function mbw($method) {
return view('payment', ['type' => 'mbw']);
}
private function dd($method) {
return view('payment', ['type' => 'dd', 'data' => $method]);
}
public function render($type, $data) {
try {
return $this->$type($data);
} catch (Exception $e) {
throw new Exception('some error in methods');
}
}
}
rewrite Services/PaymentService.php
class PaymentService implements PaymentServiceInterface {
private $externalPaymentService;
public function __construct(ExternalPaymentService $externalPaymentService)
{
$this->externalPaymentService = $externalPaymentService;
}
public function run(PaymentRequest $request) {
try {
$tkey = uniqid(Auth::id(), true);
$this->cartCollection();
$result = $this->externalPaymentService->collect($request, $tkey)->send($request, $tkey);
$this->addToPreOrdersTable($tkey);
$status = $result['status'];
if ($status != 'ok') {
throw new Exception('Bad request');
}
return (new PaymentsRenderStrategy())->render($request->type, ($result['method']);
} catch (Exception $e) {
return back()->withInput()->withErrors(['Error' => 'An error has occured.']);
}
}
protected function addToPreOrdersTable(string $hash)
{
$this->cartCollection();
OrderService::createPreOrder($this->contents, $hash, 'buy');
}
}
Validateon we moved in PaymentRequest and we don't need extra checks like if (in_array($request->type, $availableMethods)).
$this->$type($data); - call need method by type (note: use $this->$type)
also we took the logic out of ExternalPaymentService and serparate method send by collect and send
ExternalServices/PaymentExternalService.php
class ExternalPaymentService {
private $paymentBody;
public function collect(PaymentRequest $request, $tkey) {
$this->paymentBody = [
"method" => $request->type,
"value" => (float)\Cart::getTotal(),
"currency" => "EUR",
"key" => $tkey,
"type" => "sale",
"customer" =>
[
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "",
],
"capture" =>
[
"descriptive" => "Order",
"transaction_key" => $tkey,
],
];
if ($request->type == 'dd') {
$sdd_mandate = ['sdd_mandate' => [
"iban" => $request->iban,
"account_holder" => Auth::user()->name,
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "0000000",
"country_code" => "EU",
]];
$this->paymentBody = array_merge($this->paymentBody, $sdd_mandate);
}
return $this;
}
public function send()
{
$response = Http::withHeaders([
'AccountId' => config('payment.accountId'),
'ApiKey' => config('payment.apiKey'),
'Content-Type' => 'application/json',
])->post('REMOVED API LINK', $this->paymentBody);
return $response->collect();
}
}
Here we refactor I hope helped you. Next stage: take out magic constants (cc,mb,mbw,dd), to short methods ...etc. See more
There are two ways to reduce duplicate POST request code in Laravel/PHP for a payment system:
- Use a trait. This allows you to define a reusable block of code that can be shared across multiple controllers.
- Use a separate class. This allows you to encapsulate the common code in a single place, making it easier to maintain and update.
The approach you choose depends on your specific needs and preferences. If you need to reuse the code in a small number of controllers, a trait may be sufficient. If you need to reuse the code in a large number of controllers or if the code is complex, a separate class may be a better choice.
Here is an example of how to use a trait to reduce duplicate POST request code:
<?php
namespace App\Controllers\Traits;
use Illuminate\Http\Request;
trait PaymentRequestTrait
{
protected function createRequest(Request $request)
{
$this->cartCollection();
$availableMethods = ['mb', 'mbw', 'cc', 'dd'];
$tkey = uniqid(Auth::id(), true);
if (in_array($request->type, $availableMethods)) {
$payment_body = [
"method" => $request->type,
"value" => (float)\Cart::getTotal(),
"currency" => "EUR",
"key" => $tkey,
"type" => "sale",
"customer" =>
[
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "",
],
"capture" =>
[
"descriptive" => "Order",
"transaction_key" => $tkey,
],
];
if ($request->type == 'dd') {
$sdd_mandate = ['sdd_mandate' => [
"iban" => $request->iban,
"account_holder" => Auth::user()->name,
"name" => Auth::user()->name,
"email" => Auth::user()->email,
"phone" => isset($request->telm) ? $request->telm : "0000000",
"country_code" => "EU",
]];
$payment_body = array_merge($payment_body, $sdd_mandate);
}
$response = Http::withHeaders([
'AccountId' => config('payment.accountId'),
'ApiKey' => config('payment.apiKey'),
'Content-Type' => 'application/json',
])->post('REMOVED API LINK', $payment_body);
$this->addToPreOrdersTable($tkey);
return $response->collect();
}
}
protected function addToPreOrdersTable(string $hash)
{
$this->cartCollection();
OrderService::createPreOrder($this->contents, $hash, 'buy');
}
}
To use the trait, simply add it to the top of each controller that needs to create a payment request:
<?php
namespace App\Controllers;
use App\Controllers\Traits\PaymentRequestTrait;
class PaymentController extends Controller
{
use PaymentRequestTrait;
public function index(Request $request)
{
// ...
}
}
<?php
namespace App\Controllers;
use App\Controllers\Traits\PaymentRequestTrait;
class PaymentExpressController extends Controller
{
use PaymentRequestTrait;
public function index(Request $request)
{
// ...
}
}
I would create another service layer that would handle the creation of the request and maybe the call to the third party api. Your controllers are there to create the best response to the http request not really to create the best response, create external requests, handle them, etc..