1
 foreach ($remote_user_detail as $value){
        $user = User::where('unique_id',$value->userid)->first();

        if(count($user) > 0){
            $user->unique_id = $value->userid;
            $user->user_first_name = $value->name;
            $user->card_id = $value->cardnumber;
            $user->save();
        }else{
            $user = new User();
            $user->company_id = Auth::user()->company_id;
            $user->user_label = 2;
            $user->unique_id = $value->userid;
            $user->user_first_name = ($value->name=='') ? '' : $value->name ;
            $user->card_id = $value->cardnumber;
       $user->save();
}
}

now how should i optimize this if else because unique id user_first_name and card number are same

4
  • 1
    How about just putting the different bit in the if() ... else... rather than all of the code? Commented Jun 19, 2018 at 6:00
  • how can you elaborate Commented Jun 19, 2018 at 6:01
  • but if name is null then it should take user_first_name as null and if there is any name then it should take that name instead. Commented Jun 19, 2018 at 6:02
  • 2
    So as pointed out in one of the answers - you may as well just assign the value of the name without the if altogether. Commented Jun 19, 2018 at 6:06

7 Answers 7

6

According to your if check there is no need for an if check because if $value->name == '' you are saving empty string in $user->user_first_name = '', so you can remove if check

$user = new User();
$user->company_id = Auth::user()->company_id;
$user->user_label = 2;
$user->unique_id = $value->userid;
$user->user_first_name = $value->name;
$user->card_id = $value->cardnumber;
$user->save();

Edit for updated code base you could refactor your code as

foreach ($remote_user_detail as $value){
    $user = User::where('unique_id',$value->userid)->first();
    if(count($user) == 0){
        $user = new User();
        $user->company_id = Auth::user()->company_id;
        $user->user_label = 2;
        $user->unique_id = $value->userid;
    }
    $user->user_first_name = $value->name;
    $user->card_id = $value->cardnumber;
    $user->save();
}
Sign up to request clarification or add additional context in comments.

2 Comments

but in my table user_first_name is 'NOT NULL' so there should some value to be given
@Rahul if you have to put some value then you will need an if statement , Also what is your default value for user_first_name ?
2

As Khalid pointed out you don't need to check as you're already assigning empty value so it's a waste. This solution is only intended If you want to add anything else.

Just use Ternary opertor on the $user->user_first_name like

            $user = new User();
            $user->company_id = Auth::user()->company_id;
            $user->user_label = 2;
            $user->unique_id = $value->userid;
            $user->user_first_name = ($value->name=='') ? '' : $value->name ;
            $user->card_id = $value->cardnumber;
            $user->save();

Comments

1

In a if statement i would put only the things which are changing according if condition. There you can apply the DRY principle.

Follow this link for more details http://web-techno.net/dry-principle-explained/

In your case I will go for the ternary operator:

If this is new for you, check this https://davidwalsh.name/php-shorthand-if-else-ternary-operators

$user = new User();
$user->company_id = Auth::user()->company_id;
$user->user_label = 2;
$user->unique_id = $value->userid;
$user->user_first_name = $value->name == '' ? '' : $value->name
$user->card_id = $value->cardnumber;
$user->save();

Comments

1

May be like this ?

$user = new User();
$user->company_id = Auth::user()->company_id;
$user->user_label = 2;
$user->unique_id = $value->userid;
$user->card_id = $value->cardnumber;
if($value->name=='') {
    $user->user_first_name = '';
}else {
    $user->user_first_name = $value->name;
}
$user->save();

Comments

0

I recommend you create a User constructor, and just pass in the user as parameter 1, and $value as parameter 2

class User {
    //definitions here
    function __construct2($user,$value) 
    { 
        company_id = $user->company_id;
        user_label = 2;
        unique_id = $value->userid;
        user_first_name = $value->name;
        card_id = $value->cardnumber;
    }
}

This highly cleans up the code for future use, if wanting to reallocate something like this.

$user = new User(Auth::user(), $value);

Also, as shown -- no if statement is needed

Comments

0

How I see in 2 condition $user->user_first_name is always equal to $value->name. See your self if($value->name=='') {$user->user_first_name = ''} So just write your code without if else.

$user = new User();
$user->company_id = Auth::user()->company_id;
$user->user_label = 2;
$user->unique_id = $value->userid;
$user->user_first_name = $value->name;
$user->card_id = $value->cardnumber;
$user->save();

Also, one advice for the feature if you have if else situation use PHP 7 Null coalescing operator here is the link and example

http://php.net/manual/en/migration70.new-features.php#migration70.new-features.null-coalesce-op

// Fetches the value of $_GET['user'] and returns 'nobody'
// if it does not exist.
$username = $_GET['user'] ?? 'nobody';
// This is equivalent to:
$username = isset($_GET['user']) ? $_GET['user'] : 'nobody';

// Coalescing can be chained: this will return the first
// defined value out of $_GET['user'], $_POST['user'], and
// 'nobody'.
$username = $_GET['user'] ?? $_POST['user'] ?? 'nobody';

Comments

0
 foreach ($remote_user_detail as $value){
            $user = User::where('unique_id',$value->userid)->first();

            if(count($user) > 0){
                $user->unique_id = $value->userid;
                $user->user_first_name = $value->name;
                $user->card_id = $value->cardnumber;
                $user->save();
            }else{
                $user = new User();
                $user->company_id = Auth::user()->company_id;
                $user->user_label = 2;
                $user->unique_id = $value->userid;
                $user->user_first_name = ($value->name=='') ? '' : $value->name ;
                $user->card_id = $value->cardnumber;
           $user->save();
}
}

now how should i optimize this if else because unique id user_first_name and cardnumber are same

2 Comments

I guess you should update your post or ask a new question including updated code, do not post this as an asnwer
@junaid i have updated my question can you help me out.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.