nafiesl / silsilah

A genealogy/family tree application, built with Laravel.
MIT License
600 stars 289 forks source link

Refactor couple and user policy #106

Closed azharlihan closed 1 year ago

azharlihan commented 1 year ago

Refactor this and this because there is 4 condition in one if statement. I think it's more readable and more debuggable to split to multiple if statement instead wrap it on a single if separated by or operator.

Refactor this because it's shorter syntax to use eloquent properties instead of eloquent relation method. It's also give efficient query when in the controller or view we need user's husband data user's wife data, eloquent will not query to the database again. Also i removed unused key in foreach statement.

azharlihan commented 1 year ago

This PR will ready for review if PR #89 and #105 approved and have merged.

nafiesl commented 1 year ago

Hello @azharlihan, thanks for this Refactor PR. After checking on your changes:

I think it's more readable and more debuggable to split to multiple if statement instead wrap it on a single if separated by or operator.

Yes, but multiple if statement seems too long for me. I prefer this way. https://github.com/nafiesl/silsilah/blob/1334f3145c05875f941e13cb7e76cf5065bdaa9c/app/Policies/UserPolicy.php#L21-L29

Refactor this because it's shorter syntax to use eloquent properties instead of eloquent relation method. It's also give efficient query when in the controller or view we need user's husband data user's wife data, eloquent will not query to the database again. Also i removed unused key in foreach statement.

OK I get the idea, I would prefer this way for that part:

$user->husbands->pluck('id')->contains($editableUser->id)
$user->wifes->pluck('id')->contains($editableUser->id)

Well I think we have already done the refactoring for the couple and user policy object. I believe we can close this PR.

Thank you, @azharlihan