PHP Laravel: A better way than these if statements

Question

I need to assign commissions to a newly created price. Commissions are morphable for clients, types and prices. so if there is a type commission it should get it first, fallback to clients, fallback to default.

my code works, but it feels a bit too "if-y". maybe there is a better approach to this?

  private function addDefaultOnlineCommission(Price $price)
   {
       $defaultCommission = (object)Commission::DEFAULT_COMMISSIONS;

       $typeCommission = $price->type->commissions()
           ->where('is_online', '=', true)->first();

       $clientCommission = $price->type->client->commissions()
           ->where('is_online', '=', true)->first();


       if (!$clientCommission && !$typeCommission) {
           $commission = $defaultCommission;
       }

       if ($clientCommission && !$typeCommission) {
           $commission = $clientCommission;
       }

       if ($typeCommission) {
           $commission = $typeCommission;
       }

       $price->commissions()->create([
           'commission_type'   => $commission->commission_type,
           'commission_value'  => $commission->commission_value,
           'min_value'         => $commission->min_value,
           'is_online'         => true,
           'valid_from'        => Carbon::now()->format('Y-m-d H:i:s'),
       ]);
   }

I'd say use elseifs, but anything beyond that might get a bit convoluted. A question like this can tend towards opinionation, but it might be better over at codereview — aynber
– aynber, Commented Dec 10, 2018 at 16:51
thanks. you are right about opinionation, but also in codereview you will get only one opinion :) — Michael Ploeckinger
– Michael Ploeckinger, Commented Dec 10, 2018 at 16:59

Devon Bessemer · Accepted Answer · 2018-12-10 16:55:07Z

6

Purely opinionated but I'd handle it with null coalescing operators:

$commission = $typeCommission ?? $clientCommission ?? $defaultCommission;

This will use the first non-null value. So it sets a precedence from left to right.

answered Dec 10, 2018 at 16:55

Devon Bessemer

35.6k10 gold badges77 silver badges102 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

apokryfos Over a year ago

You can also coalesce the actual queries to avoid making the 2nd one if the first one works.

Fitzi · Accepted Answer · 2018-12-10 17:33:01Z

2

I think the functions looks to "if-y" because your statements are more complicated than they have to be. Also you are always loading all of your fallbacks, which isn't neccessary in every case. I would refactor the function to only load the next level if necessary. This could look something like this:

private function addDefaultOnlineCommission ( Price $price ) {

    // load the type commission
    $commission = $price->type->commissions()->where( 'is_online', '=', true )->first();

    // if there is no type commission, check client commission
    if( empty($commission) ){
        $commission = $price->type->client->commissions()->where( 'is_online', '=', true )->first();
    }

    // if there is no client commission either, fall back to the default
    if( empty($commission) ){
        $commission = (object)Commission::DEFAULT_COMMISSIONS;
    }

    $price->commissions()->create( [
        'commission_type'  => $commission->commission_type,
        'commission_value' => $commission->commission_value,
        'min_value'        => $commission->min_value,
        'is_online'        => true,
        'valid_from'       => Carbon::now()->format( 'Y-m-d H:i:s' ),
    ] );

}

This does not only reduce the length of your function, it also makes it more readable imho. Additionally you avoid unnecessary queries.

Note: You can replace empty() with is_null() as first() returns null on empty results.

edited Dec 10, 2018 at 17:33

answered Dec 10, 2018 at 17:25

Fitzi

1,67211 silver badges19 bronze badges

1 Comment

Michael Ploeckinger Over a year ago

Thank you. that looks like a good approach to avoid queries also.

Vũ Anh Lê · Accepted Answer · 2018-12-10 16:54:19Z

1

How about this?

   if (!$clientCommission && !$typeCommission) {
       $commission = $defaultCommission;
   } else if ($clientCommission && !$typeCommission) {
       $commission = $clientCommission;
   } else {
       $commission = $typeCommission;
   }

answered Dec 10, 2018 at 16:54

Vũ Anh Lê

111 bronze badge

Collectives™ on Stack Overflow

PHP Laravel: A better way than these if statements

3 Answers 3

1 Comment

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related