Laravel transactions with a database

Question

I'm really concerned about my transaction with a database and my code structure. I have created more methods to insert, update, delete data to the database but sometimes some of them will break or miss. That's why I try to use Laravel transaction and PHP catch and throw exceptions to rollback and avoid any errors happening during those method processes.

By they way, I don't yet understand how try/catch and Laravel transaction catch those errors and return or throw to the end user.

However, I've tried to find all kind of errors will happen and how I can throw those error types to the end users in the catch block.

This function work find but I want to check if any errors happen and how I can return those type of errors to the end user by using Laravel transaction and try.

public function Dosomething($id)
    {
     if (Request::ajax()) {    
        $data = [
          'till_account_id' => Request::input('till_account_id'),
          'from_account' => Request::input('from_account'),
          'to_account' => Request::input('to_account'),
          'till_user_id' => Request::input('till_user_id'),
          'branch_id' => Request::input('branch_id'),
          'operate_by' => Request::input('operate_by'),
          'type' => Request::input('type'),
         'tranx_time' => date('Y-m-d H:m:s', time())
       ];
      DB::beginTransaction();
     try {
         $update = Teller::where('id', '=', $id)->update(['balance' => Request::input('balanceCommision')]);
         if (!empty($update)) {
             $data['cash_out'] = Request::input('cash_out');
             $trans_cashIn = DB::table('till_transaction')->insertGetId($data);

             $data['cash_in'] = Request::input('cash_in');
             $data['cash_out'] = 0;
             $trans_cashOut = DB::table('till_transaction')->insertGetId($data);
        }
            $test = DB::commit();
            return ['up' => $update, 'insCashIn' => $trans_cashIn, 'insCashOut' => $trans_cashOut, 'data' => $data, 'cash_out' => $data['cash_out'],'commit'=>$test];
       } catch (Exception $e) {
           DB::rollBack();
           throw $e;
      }
   }
}

Answer 1

Overall, you've got decent structure there. However, there are a few things I can see from my limited experience.

---

You defined an associative array $data at the beginning. From experience, it usually works best to define it fully up front instead of in bits and pieces. This is not required in PHP like it is in Java, but it usually works better because it keeps your code looking clean and organized. It's hard to know what exactly is in your array if you add to it in multiple places.

From what I can see, you don't have any specific delimiters or checks to see if the inputs exist, this should be the first thing you do. If something is null, there should be red flags going up.

That being said, inside the try statement you insert into the database twice. Both times you pass $data to the insertGetId() magic method. While you do want to pass an associative array, you must make sure that all required fields are in that array. Effectively, you're duplicating your data with just an extra column.

Reading your $data array, you have a column named 'till_account_id'. If I understand the project correctly, this field should be unique as no one person should share an account. There may be cosigners, but it isn't technically their account. That being said, I think the 'till_account_id' should be your primary key, which means your insert should look like this:

DB::table('till_transaction')->insertGetId($data,'till_account_id');

---

Be very careful when you return data. You return several variables defined inside of an if statement. If the update fails, it will return null which means !empty($update) will fail because you're trying to read a null object. The same goes for further down, when you return, all the values that were defined inside the if statement wouldn't exist, thus returning null.

---

Good job catching the exception and then rethrowing it to the caller. Most people would catch it and forget to handle it. That being said, though, depending on how specific of an app you're developing, it might be beneficial to make a custom exception class and do something like this:

catch (Exception $e) {
    //respond here as needed
    throw new CustomException('some message describing error',0,$e);
}

This would do whatever you need to do, then rethrow the exception in a more manageable fashion. You would have a message specific to what you need to explain - details are very important in debugging, you would have the error code, and you would have the previous exception passed to the new exception for the trace.

Answer 2

I'd like to add some additions to @Trojan404's answer which I believe are most important points to be focused on, especially because this is the Code Review section :)

1. First of all, please use camelCase for naming functions. That is preferred as a common practice nowadays.
2. If the request is not AJAX, you are not throwing any exceptions and therefore you'll still get a HTTP Status Code of 200, (with a blank body ofcourse)
3. Avoid using Request multiple times and prefer common functions of Request like all() or only() or except() or get() to catch request variables.
4. Please prefer classes like Carbon for date based use cases.
5. Finally, I don't find any problem in you try/catch block... But like @Trojon404 said, you can also your CustomException... such as TransactionFailedException and use your Handler to handle according to your needs.

I was able to convert your code to this. Please use proper spacing wherever possible. Practicing and writing cleaner code must be the top priority for a developer... :)

public function doSomething($id)
{
  if(!request()->ajax()) {
    throw new BadRequestException;
  }

  $inputs = request()->all();

  $data = array_only([
    'till_account_id', 'from_account', 'to_account', 'till_user_id', 'branch_id', 'operate_by', 'type'
  ], inputs);

  $data = Carbon::now()->toDateTimeString();

  DB::beginTransaction();

  try {
    $update = Teller::where('id', $id)->update([
      'balance' => $inputs['balanceCommision']
    ]);

    if($update) {
      $data['cash_out'] = $inputs['cash_out'];
      $trans_cashIn = DB::table('till_transaction')->insertGetId($data);

      $data['cash_in'] = $inputs['cash_in'];
      $data['cash_out'] = 0;
      $trans_cashOut = DB::table('till_transaction')->insertGetId($data);
    }

    $test = DB::commit();

    return [
      'up' => $update,
      'insCashIn' => $trans_cashIn,
      'insCashOut' => $trans_cashOut,
      'data' => $data,
      'cash_out' => $data['cash_out'],
      'commit'=>$test
    ];
  } catch (Exception $e) {
    DB::rollBack();

    throw $e;           // or maybe something like `throw new TransactionFailedException`;
  }
}