Return early if the AuthPolicy being filtered is marked for deletion #632
Conversation
controllers/authpolicy_authconfig.go
Outdated
| affectedPolicies = utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { | ||
| p, ok := policy.(*api.AuthPolicy) | ||
| if !ok { | ||
| return false | ||
| } | ||
| if p.DeletionTimestamp != nil { | ||
| return false | ||
| } | ||
| return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && | ||
| ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() |
There was a problem hiding this comment.
| ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() | |
| ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() && p.DeletionTimestamp == nil |
?
There was a problem hiding this comment.
So I did have it that way to start. My reasoning for not doing it was why do the extra function calls when it can return early if there is a deletion time stamp.
There was a problem hiding this comment.
Make it the first condition then?
| ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() | |
| return p.DeletionTimestamp == nil && | |
| kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && | |
| ap.GetUID() != policy.GetUID() && | |
| p.IsAtomicOverride() |
I'm usually in favour of safeguard clauses but in this case where the return is straightforward, the less ifs the better, no? In fact, if really we want to push it:
| ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() | |
| return ok && | |
| p.DeletionTimestamp == nil && | |
| kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && | |
| ap.GetUID() != policy.GetUID() && | |
| p.IsAtomicOverride() |
There was a problem hiding this comment.
Moving the ok check here makes more sense also. I am kinda unsure about the readability, personally I don't like to have logic in return statements. It makes using a debugger harder. But in this case this might be the cleaner option.
Improves filter function and returns early when searching for AuthPolicies attached to the Gateway.