Skip to content

[5.8] Use Null coalescing operator to refactor - #27955

Merged
taylorotwell merged 1 commit into
laravel:5.8from
richardkeep:patch-1
Mar 21, 2019
Merged

[5.8] Use Null coalescing operator to refactor#27955
taylorotwell merged 1 commit into
laravel:5.8from
richardkeep:patch-1

Conversation

@richardkeep

Copy link
Copy Markdown
Contributor

No description provided.

@richardkeep richardkeep changed the title Use Null coalescing operator to refactor [5.8] Use Null coalescing operator to refactor Mar 21, 2019
@taylorotwell
taylorotwell merged commit 427365a into laravel:5.8 Mar 21, 2019
return $this->convertedFiles
? $this->convertedFiles
: $this->convertedFiles = $this->convertUploadedFiles($files);
return $this->convertedFiles = $this->convertedFiles ?? $this->convertUploadedFiles($files);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this not be shortened further to just return $this->convertedFiles ?? $this->convertUploadedFiles($files);?

@richardkeep richardkeep Mar 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we also want to set the value for $this->convertUploadedFiles($files) to $this->convertedFiles if it is falsy before returning.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardkeep Gotcha 🙂

@4refael

4refael commented Mar 23, 2019

Copy link
Copy Markdown
Contributor

This change may have side effects as it's not identical to the original behavior.
The correct operator to use here is the tenary operator:
return $this->convertedFiles ?: $this->convertedFiles = $this->convertUploadedFiles($files);

@driesvints

Copy link
Copy Markdown
Member

@rellect I think it's the same result in the end, right? Although your solution makes a bit more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants