Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Eloquent::when() – no more if-else’s #44

Merged
merged 16 commits into from
Dec 27, 2019

Conversation

RoisNewVersion
Copy link
Contributor

use Eloquent::when() method instead of using if else statement.

@nafiesl
Copy link
Owner

nafiesl commented Dec 22, 2019

@RoisNewVersion, thanks for the PR. Could you please fix your code to pass the Style-CI? I see you change a lot of string concatenations.

@RoisNewVersion
Copy link
Contributor Author

Sorry for interrupt, I used to have php formater ext in my VSCode, I just hit Shift+Alt+F that automatically reformat the code.
as I see, the styleci didn't accept white space between string concatenation, am I right? So, what should I do then?

@nafiesl
Copy link
Owner

nafiesl commented Dec 22, 2019

@RoisNewVersion, you can turn off your auto format from your VSCode, then just change the lines that you want to change.

If we passed the style-ci, I will start reviewing your changes.

@RoisNewVersion
Copy link
Contributor Author

Okay, I did it.

Copy link
Owner

@nafiesl nafiesl left a comment

Choose a reason for hiding this comment

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

@RoisNewVersion, I see some short variable names, those variable names are not very readable. I am suggesting some changes to this PR.

app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
app/Http/Controllers/Controller.php Outdated Show resolved Hide resolved
RoisNewVersion and others added 14 commits December 22, 2019 21:12
Ahh.. I see, my silly mistake. Thanks for suggestion

Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
Co-Authored-By: Nafies Luthfi <nafiesl@gmail.com>
@nafiesl
Copy link
Owner

nafiesl commented Dec 27, 2019

@RoisNewVersion, why are you closing this PR?

Copy link
Owner

@nafiesl nafiesl left a comment

Choose a reason for hiding this comment

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

Looks great! I am merging this PR @RoisNewVersion. Thanks.

@nafiesl nafiesl merged commit 6031edc into nafiesl:master Dec 27, 2019
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.

2 participants